Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4001 - Datatable use Datagrid - Editable borders corrections #4230

Conversation

yaguzmang
Copy link
Contributor

Fixing cell border rendering when adjacent cells are non-editable:

approach.2.mp4

The first approach which is consistent with how the borders are currently done (top,left for each cell) doesn't quite work. Since some cells span multiple columns, setting the border top for those can show and editable border section where it shouldn't be:

approach.1.mp4

Also, because some cells have a non-transparent background color, the outline of the focused editable cell is hidden. Fixing that with z-index wasn't possible because the sticky headers should always have a higher z-index.

Due to that, the best approach was to have some double borders, which doesn't create those issues and helps distinguish more clearly editable vs readonly cells.

@yaguzmang yaguzmang self-assigned this Jan 18, 2025
@yaguzmang yaguzmang requested a review from minotogna January 18, 2025 01:26
@yaguzmang yaguzmang marked this pull request as ready for review January 18, 2025 01:26
@@ -79,6 +79,10 @@ $highlight-top-bottom: inset 0 1px $ui-accent, inset 0 -1px $ui-accent;
border-left: $border-editable;
border-top: $border-editable;

&.bottomEditable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's call those classes lastEditableRow, lastEditableCol (and add them next to lastCol ,...)

@mergify mergify bot merged commit 970364e into 4001-datatable-use-datagrid Jan 21, 2025
5 checks passed
@mergify mergify bot deleted the 4001-datatable-use-datagrid-editable-borders branch January 21, 2025 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants