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.

Sign up for GitHub

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

DataViews: Add space for focus outline to primary field #59068

Closed
wants to merge 1 commit into from
Closed

DataViews: Add space for focus outline to primary field #59068

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/dataviews/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,10 @@
display: block;
width: 100%;

&:has(a) {
Copy link
Contributor

@ntsekouras ntsekouras Feb 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think the misalignment with the padding won't be great in general.. Maybe we could remove the overflow: hidden; in the main container above? 🤔 --cc @jameskoster

In general it makes me wonder what styles should belong in the DataViews package and which ones should be added by the consumers.. Maybe the link styles should be provided by the consumer if needed, because it could result to unexpected results..

For example in primary field someone could have a paragraph with nested links(a tag). Currently with ship with display: block; styles and would be weird IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree this could lead to inconsistencies in a single data view which isn't ideal. It also breaks the 4px spacing system.

As for whether the styles are built-in, I see that as an implementation detail best left to y'all engineers to decide :) I can see that built-in styles could be unhelpful in some situations. As long as it's possible to apply truncation to page / template / pattern titles in certain layouts as required I'm happy.

Either way, the issue this PR aims to fix will continue to exist. @t-hamano perhaps an alternative approach could be to add 4px horizontal padding to .dataviews-view-grid__primary-field and reduce the gap on dataviews-view-grid__title-actions from 8 to 4px?

Copy link
Member

Choose a reason for hiding this comment

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

I've got a PR that touches on the same thing but doesn't need to add the padding #58814

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oandregal I had overlooked that PR 😅 #58814 seems to have a broader discussion going on, so I'm happy to close this PR to prevent cluttering the discussion.

padding: 0 2px; // space for focus outline to appear.
}

a {
text-decoration: none;
color: inherit;
Expand Down
Loading