SkipNavigation improvements #4739
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LLM Analysis of PR ChangesSummaryThis PR introduces an Key Points to Review
Style & Consistency
|
|
Storybook staging is available at https://kiwicom-orbit-avi-skip-nav.surge.sh Playroom staging is available at https://kiwicom-orbit-avi-skip-nav.surge.sh/playroom |
Ajuska
commented
May 5, 2025
|
Size Change: +44 B (+0.01%) Total Size: 464 kB
ℹ️ View Unchanged
|
Deploying orbit with
|
| Latest commit: |
19e681c
|
| Status: | ⚡️ Build in progress... |
| @@ -80,6 +80,22 @@ export const Default: Story = { | |||
| }, | |||
| }; | |||
|
|
|||
| export const InNavigationBar: Story = { | |||
| render: () => ( | |||
| <> | |||
There was a problem hiding this comment.
As this story should display SkipNavigation usage inside NavigationBar, let's use NavigationBar component here? I would also add isInNav to controls.
There was a problem hiding this comment.
Made the edits, lmk if you'd prefer to add more controls fixup! feat(SkipNavigation): add isInNav prop for styling in navigation (will autosqash this commit later, it's for you to review it more easily)
There was a problem hiding this comment.
I like that you used the table to differentiate between SkipNavigation and NavigationBar props 👍
I think the controls you have are fine, tbh I thought you would just add isInNav and transparentBgAtTop and hardcode the rest 😅 We can keep all of them but I would probably move the args from both Playground and your new story and set it on meta so it's consistent with story files for other components. Or spread the args from the Playground story.
There was a problem hiding this comment.
There was a problem hiding this comment.
I meant to move those args to meta so that there are no empty values in your new story 😅 And keep controls for Default story disables. I apologise for confusion.
We usually followed this pattern - set all args/argTypes on meta, have Default story with disabled controls, have other stories with arg values inherited from meta and alternatively some unnecessary controls excluded and lastly Playground story with all controls inherited from meta. That's why I suggested to move it so you don't have to set new empty args on InNavigationBar but they can be inherited.
There was a problem hiding this comment.
I've checked random components and they differ - for example Button and CallOutBanner use controls also for the default story so I kept it also for this component. As for the special args I used different ones on purpose because I think in the header it will mostly be used in this simple form.
|
Regarding commits, I would say |
No, I think the type should remain as "fix". This fixes missing accessibility attributes and "fix" should be used for this type of issues too. "chore" is for maintenance tasks that don't modify source code or user-facing functionality. |
Ajuska
force-pushed
the
avi/skip-nav
branch
2 times, most recently
from
f6092ed to
e433bf8
Compare
|
What happened with all those changed snapshots? |
@domihustinova the test were failing so I've merged the changes from master branch and let the github generate those so the changes are expected due to different macOs (the previous one were generated locally on my machine) |
domihustinova
approved these changes
May 9, 2025
![callstackai[bot]](https://avatars.githubusercontent.com/in/1040204?s=60&v=4){kind=small}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes FEPLT-2453
✨
Description by Callstackai
This PR improves the SkipNavigation component by adding new props for navigation integration and updating the documentation accordingly.
Diagrams of code changes
sequenceDiagram participant User participant SkipNavigation participant NavigationBar participant Select Note over SkipNavigation: New isInNav prop added alt isInNav=true User->>NavigationBar: Tab/Focus NavigationBar->>SkipNavigation: Render with nav styling SkipNavigation->>Select: Show section selector<br/>(max-width: 250px) else isInNav=false User->>SkipNavigation: Tab/Focus SkipNavigation->>Select: Show section selector<br/>(max-width: 800px) end Select-->>User: Select section Note over User,Select: Navigation options:<br/>1. Jump to section<br/>2. Common actions<br/>3. Send feedbackFiles Changed
isInNavprop.isInNavprop.isInNavprop.isInNavprop.isInNavprop for styling.isInNavto the Props interface.This PR includes files in programming languages that we currently do not support. We have not reviewed files with the extensions
.md,.mdx. See list of supported languages.