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

Add rounded corners to all components #5938

Merged
merged 1 commit into from
Nov 8, 2023
Merged

Add rounded corners to all components #5938

merged 1 commit into from
Nov 8, 2023

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Nov 6, 2023

This PR adds preliminary support for rounded corners in the client, disabled behind a feature flag.

The changes include:

  • Update to the latest frontend-shared version, which introduces support for rounded corners.
  • Adjust all local components so that they render with rounded corners when the feature flag is enabled.
  • Replace hardcoded rounded corners (as in rounded-[4px]) to use standard rounded corner sizes.

Testing steps

  1. Check out this branch
  2. Go to http://localhost:3000 and verify there's no visual changes and everything looks mostly the same.
    • There are some exceptions and small adjustments, like dropdown menus where the arrow has a bit of space from the right.
  3. Make sure your local h instance is up to date. Go to http://localhost:5000/admin/features and enable the rounded_corners feature flag.
  4. Repeat step 2. Now all components should have more evident rounded corners.

Closes #5602

@@ -69,6 +69,7 @@ export default function SidebarPanel({
onClose={closePanel}
transitionComponent={Slider}
variant={variant}
scrollable={false}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to check why, but making the Dialog scrollable messes with the bottom corners and makes them non-rounded.

This specific Dialog does not have a fixed height, so the scroll does not add any value here and we can just disable it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lg: '0.5rem',
},
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes annotator components be always rounded.

This was their behavior already, but it was set via hardcoded border radius rounded-[4px].

Those classes have been replaced by rounded, and this config ensures they keep looking the same regardless the feature being enabled or not, while it becomes ready for the future, when we just use tailwind defaults.

borderRadius: {
// Equivalent to tailwind default `rounded-sm` size
DEFAULT: '0.125rem',
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now use the config defined in frontend-shared's tailwind preset.

@acelaya acelaya force-pushed the rounded-corners branch 2 times, most recently from 76ae2a4 to 0066570 Compare November 7, 2023 09:02
Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Merging #5938 (6aaaa6c) into main (07b9710) will increase coverage by 0.00%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5938   +/-   ##
=======================================
  Coverage   99.44%   99.44%           
=======================================
  Files         257      257           
  Lines        9887     9888    +1     
  Branches     2362     2363    +1     
=======================================
+ Hits         9832     9833    +1     
  Misses         55       55           
Files Coverage Δ
src/annotator/components/AdderToolbar.tsx 100.00% <ø> (ø)
src/annotator/components/ClusterToolbar.tsx 100.00% <ø> (ø)
src/annotator/components/Toolbar.tsx 100.00% <ø> (ø)
...components/Annotation/AnnotationPublishControl.tsx 100.00% <ø> (ø)
...r/components/Annotation/AnnotationShareControl.tsx 100.00% <ø> (ø)
src/sidebar/components/AutocompleteList.tsx 100.00% <ø> (ø)
src/sidebar/components/MarkdownEditor.tsx 94.84% <ø> (ø)
src/sidebar/components/Menu.tsx 100.00% <ø> (ø)
src/sidebar/components/MenuItem.tsx 100.00% <ø> (ø)
src/sidebar/components/PaginationNavigation.tsx 100.00% <ø> (ø)
... and 3 more

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@@ -130,9 +130,10 @@ export default function ClusterToolbar({
}

return (
<Card>
<Card classes="overflow-hidden">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -107,11 +107,11 @@ export default function AutocompleteList<Item>({
)}
data-testid="autocomplete-list-container"
>
<Card width="auto">
<Card width="auto" classes="overflow-hidden">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -239,7 +239,7 @@ export default function MenuItem({
const wrapperClasses = classnames(
'focus-visible-ring ring-inset',
'w-full min-w-[150px] flex items-center select-none',
'border-b',
'border-b rounded-none cursor-pointer',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This addresses inconsistencies in which link menu items were being rendered with rounded corners on hover, while non-links were not rounded.

I took the opportunity to address other inconsistencies between link vs non-link menu items, like hover color and cursor type.

// appropriate times.
const withSaveMessage = saveCount > 0 && !loading;

return (
<Card data-testid="profile-container">
<Card data-testid="profile-container" classes="overflow-hidden">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robertknight
Copy link
Member

Overall this looks good. One visual issue I see is extra scrollbar gutters on the right and bottom edges of the sidebar in Chrome:

Scrollbar gutters

In Safari I only see these appear if the current tab has enough annotation cards to fill the viewport.

Somer small issues:

  1. I'm seeing the rounding on the right of the Post button being out of sync with the rounding on the left:
Post menu
  1. The way the rounding of the top-right corner of the menu blends with the up-arrow looks a bit odd when the up arrow is close to the edge. The simplest fix would be to square off the top corners initially. Maybe have a look at some other web properties to see how they handle this in their design.
Menu top-right rounding

Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

Overall I think this PR is good except for the extra scrollbar gutters that have appeared. I mentioned a couple of other minor issues.

src/sidebar/components/ShareDialog/ShareDialog.tsx Outdated Show resolved Hide resolved
'right-0': align === 'right',
// Adding negative right distance so that the menu arrow does
// not overlap with the top-right corner when it's rounded
'-right-1': align === 'right',
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering whether we should remove the up arrow for menus where the up-arrow was previously aligned with either the right or left edge of its toggle button. This seems to be the pattern that several web properties (eg. GitHub) use.

@acelaya
Copy link
Contributor Author

acelaya commented Nov 8, 2023

@robertknight I have pushed some changes:

This could be simplified by factoring out the initializer for selectedTab as eg. initialTab and then testing tabbedDialog && selectedTab === initialTab

Done

I'm seeing the rounding on the right of the Post button being out of sync with the rounding on the left

Fixed

The way the rounding of the top-right corner of the menu blends with the up-arrow looks a bit odd when the up arrow is close to the edge

As discussed in slack, I'm deferring this to a follow-up PR where I will review the usage of arrow in menus in general.

One visual issue I see is extra scrollbar gutters on the right and bottom edges of the sidebar in Chrome

I think this is not something introduced by this PR 🤔

I see the sidebar has overflow-scroll, which is what I think is causing this. I have been able to reproduce it in prod and QA as well.

@acelaya acelaya requested a review from robertknight November 8, 2023 15:31
@robertknight
Copy link
Member

robertknight commented Nov 8, 2023

I see the sidebar has overflow-scroll, which is what I think is causing this. I have been able to reproduce it in prod and QA as well.

To be clear about what I was seeing. Earlier today on this branch (note lighter grey right and bottom edge):

Scrollbar gutters

vs. how what I normally see:

Prod client

The issue disappeared for me on this branch locally after switching back to it however from testing other things, and I can't see it in Chrome, Firefox or Safari after a fresh build. So looks like a false alarm, sorry for the confusion.

Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

LGTM

@acelaya acelaya merged commit 90a6adb into main Nov 8, 2023
4 checks passed
@acelaya acelaya deleted the rounded-corners branch November 8, 2023 15:50
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.

UX roundy corners
2 participants