Skip to content

Conversation

@GomezIvann
Copy link
Collaborator

@GomezIvann GomezIvann commented May 5, 2023

Checklist

  • Build process is done without errors and all tests pass in the /lib directory.
  • Self-reviewed the code before submitting.
  • Meets accessibility standards.
  • Added/updated documentation to /website as needed.
  • Added/updated tests as needed.

Description
Summary of changes in the Sidenav component:

  • sidenav.Link prop children has changed its type from string to React.ReactNode.
  • Specs updated:
    • Removed line heights to be able to properly align text inside a Sidenav link.
    • Removed unnecessary margin-right for svgs inside the compound component Title.
    • Removed React.Children from the implementation and using instead React Context API. In the new React site of documentation they set Children as a legacy API and recommend not to use it whenever possible (https://react.dev/reference/react/Children).
  • Documentation updated:
    • More logical hierarchy and improved descriptions.
    • Updated children's prop type.
    • Updated description for some props.

Closes #1587

@GomezIvann GomezIvann marked this pull request as ready for review May 5, 2023 11:30
@Jialecl Jialecl self-requested a review May 8, 2023 14:18
@Jialecl
Copy link
Collaborator

Jialecl commented May 9, 2023

The hover collapsed group title does not look correct in collapsed snapshots from chromatic.

@Jialecl
Copy link
Collaborator

Jialecl commented May 9, 2023

Also I noticed that we don't have a visual test that covers the active state.

@GomezIvann
Copy link
Collaborator Author

GomezIvann commented May 11, 2023

We are closing this pull request and continuing the work in a new one since we have made too many changes to fix a Chromatic issue. You can keep track of the work there: #1596.

@GomezIvann GomezIvann closed this May 11, 2023
@GomezIvann GomezIvann deleted the gomezivann-sidenav-updates branch June 14, 2023 14:44
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.

Change Sidenav.Link children prop type

4 participants