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

feat: add enterDelayMs-type prop to delay displaying SideNav component #13850

Merged

Conversation

Shankar-CodeJunkie
Copy link
Contributor

Closes #13791

Add enterDelayMs-type prop to Specify the duration in milliseconds to delay before displaying the sidenavigation

Changelog

Changed

  • Modified SideNav.tsx to include a new prop of type enterDelayMs to specify the duration in milliseconds to delay before displaying the sidenavigation
  • Modified UIShell.stories.js to include a playground story, where developers can test with two input params from browser. Earlier, the playground part for UIShell wasn't available, and hence I created a new one that expects two props ( isRail & enterDelayMs

Testing / Reviewing

  • Ran yarn test
  • I have also ran story book testing and modified the playground by giving different duration for prop enterDelayMs and I see the delay in opening up the sidenavigation.
image

@netlify
Copy link

netlify bot commented May 22, 2023

Deploy Preview for carbon-components-react ready!

Name Link
🔨 Latest commit ad2deb3
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/6477bed49ebb1f000884904c
😎 Deploy Preview https://deploy-preview-13850--carbon-components-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented May 22, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit ad2deb3
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/6477bed4c7b98e0008611620
😎 Deploy Preview https://deploy-preview-13850--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@Shankar-CodeJunkie
Copy link
Contributor Author

I see one of the test is failing and I am investigating the reason for it . I will push the changes, once I have a fix for it . Thanks

@sstrubberg sstrubberg requested review from tw15egan and removed request for alisonjoseph May 22, 2023 18:22
Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

Visual regressions due to class name removal, playground story respects the enter delay but it just flashes open and closes immediately.

packages/react/src/components/UIShell/SideNav.tsx Outdated Show resolved Hide resolved
@Shankar-CodeJunkie
Copy link
Contributor Author

@tw15egan : Thanks for comments. I made the required changes and push the code to the PR.

Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

I left some comments, testing locally I could achieve the same functionality without the additional classes. Let me know if I missed anything though!

packages/react/src/components/UIShell/SideNav.tsx Outdated Show resolved Hide resolved
@Shankar-CodeJunkie
Copy link
Contributor Author

Thanks @tw15egan . Yes, I removed that additional blocks in the scss file, and it worked fine without the need of additional class.

Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

Small documentation issue, but the functionality looks great. Thanks for working on this!

packages/react/src/components/UIShell/UIShell.stories.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@francinelucca francinelucca left a comment

Choose a reason for hiding this comment

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

This is looking great! I agree with @tw15egan , @Shankar-CodeJunkie could we put the controls in the sidenav rail story https://deploy-preview-13850--carbon-components-react.netlify.app/?path=/story/components-ui-shell--side-nav-rail instead of adding a playground for it? I believe adding a playground to the UIShell would involve a bigger effort than just these two controls since it's an umbrella for a lot of subcomps and we might not want to tackle that in the scope of this pr

@Shankar-CodeJunkie
Copy link
Contributor Author

@tw15egan @francinelucca : As recommended, I've removed the playground story and modified the existing
SideNav w/ Rail stories by adding the props for enterDelayMs for 1 and 3 seconds.

@francinelucca
Copy link
Collaborator

This is almost there, @Shankar-CodeJunkie we meant to add a customizable control for the enterDelayMs prop so that it's customizable in the story, here's an example with ClickableTile and the disabled prop: https://react.carbondesignsystem.com/?path=/story/components-tile--clickable

image

https://github.com/carbon-design-system/carbon/blob/main/packages/react/src/components/Tile/Tile.stories.js#L79

image

@Shankar-CodeJunkie
Copy link
Contributor Author

@francinelucca !. Oh yes .. I see it now, what needs to be changed ! .. Thanks for your help !

When I make the change, I see an extra - appears on the description column, and I m not sure how to remove it . Please let me know if there is a way to remove it. I tried to check other stories, and I am not sure, where the description is being rendered on the page

image

@tw15egan
Copy link
Collaborator

@Shankar-CodeJunkie It seems like it's having an issue pulling the description from the SideNav.propTypes definition inside SideNav.tsx. Not sure why though 🤔

Copy link
Collaborator

@francinelucca francinelucca left a comment

Choose a reason for hiding this comment

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

Not sure why it's adding the '-' in the bottom but I wouldn't worry about it, other than that LGTM

packages/react/src/components/UIShell/UIShell.stories.js Outdated Show resolved Hide resolved
packages/react/src/components/UIShell/UIShell.stories.js Outdated Show resolved Hide resolved
@Shankar-CodeJunkie
Copy link
Contributor Author

Thanks @francinelucca and @tw15egan for your help !.

@kodiakhq kodiakhq bot merged commit 4cac829 into carbon-design-system:main May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add enterDelayMs-type prop to SideNav isRail
3 participants