-
Notifications
You must be signed in to change notification settings - Fork 526
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
[UnderlineNav] : Introduce loading counters #2378
Conversation
🦋 Changeset detectedLatest commit: 516749a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
src/UnderlineNav2/LoadingCounter.tsx
Outdated
width: 22px; | ||
height: 16px; | ||
display: inline-block; | ||
border-radius: 20px;` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is 20 an arbitrary number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I got it from the CounterLabel's styles I currently don't have much design details for the loading counters except a demo on the Figma file So I tried to use CounterLabel's style for a minimal layout shift. I am not too sure about the animation colours either but I'll check it when @danielguillan is back
6ce4505
to
4567f17
Compare
4567f17
to
2e60d63
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I just left a small naming suggestion.
### Loading State for Counters | ||
|
||
```jsx live drafts | ||
<UnderlineNav counterLoading={true}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you feel about renaming this prop to loadingCounters
? I think it's a little more clear that applies to all the counters in the underline nav.
<UnderlineNav counterLoading={true}> | |
<UnderlineNav loadingCounters={true}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I debated it yesterday myself 😃 I wasn't sure but now you recommended it as well, so I'll update it, it makes more sense!
Co-authored-by: Cole Bemis <colebemis@github.com>
We need loading states for the UnderlineNav counters. See the Figma file as a ref.
Closes #1314
I wasn't sure if we should introduce a loading state to the CounterLabel component (we still can with a minor changeset) but I solved this in an isolation to UnderlineNav for now. Mainly because I needed to make something quick due to working towards Universe deadline and also there are still unknowns on the design (I couldn't find a reference to the background colours, there is only a demo video to check on the Figma file) and also the react CounterLabel and view component CounterLabel looks very different. So I thought maybe sticking to solving this in iso might be better as a start? Please let me know what you think 👀
Screen Recordings
Light Theme
layout-underlinenav-examples--counter-loading.-.27.September.2022.1.mp4
Dark Theme
layout-underlinenav-examples--counter-loading.-.27.September.2022.mp4
On coarse pointer devices (Scroll Behaviour)
layout-underlinenav-examples--counters-loading-state.-.29.September.2022.mp4
Please provide before/after screenshots for any visual changes
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.