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(PPDSC-2609): submenu overrides and href #504

Merged
merged 14 commits into from
Dec 14, 2022

Conversation

mutebg
Copy link
Contributor

@mutebg mutebg commented Dec 5, 2022

PPDSC-2609

What

  1. Background - why this is needed
  2. What did you do -
    2.1. Added new defaults to SubMenu and SubMenu items.
    2.2. Changed types so href can be passed as prop to SubMenu
    2.3. Updated Menu Context so that we know when a menu-item is in sub-menu or main menu and apply different style-preset
    2.4. Updated docs
  3. What does the reviewers should expect

I have done:

  • Written unit tests against changes
  • Written functional tests against the component and/or NewsKit site
  • Updated relevant documentation

I have tested manually:

  • The feature's functionality is working as expected on Chrome, Firefox, Safari and Edge
  • The screen reader reads and flows through the elements as expected.
  • There are no new errors in the browser console coming from this PR.
  • When visual test is not added, it renders correctly on different browsers and mobile viewports (Safari, Firefox, small mobile viewport, tablet)
  • The Playground feature is working as expected

Before:

After:

Who should review this PR:

How to test:

@github-actions github-actions bot added the feature This change contains a new feature label Dec 5, 2022
@newskit-engineering
Copy link
Collaborator

@mutebg mutebg added ready for review Please assist in getting this reviewed ready for design review Please assist in getting this reviewed labels Dec 5, 2022
@mutebg mutebg marked this pull request as ready for review December 5, 2022 06:21
@mutebg mutebg requested a review from a team as a code owner December 5, 2022 06:21
Copy link
Contributor

@baburay23 baburay23 left a comment

Choose a reason for hiding this comment

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

looks good to me my only question is..now that you added href prop to submenu is the below still needed? i noticed as well that prop href has not been added to the prop API table. Is there a reason for that? sorry maybe i am missing something!
Screenshot 2022-12-05 at 09 18 08

@mutebg
Copy link
Contributor Author

mutebg commented Dec 5, 2022

looks good to me my only question is..now that you added href prop to submenu is the below still needed? i noticed as well that prop href has not been added to the prop API table. Is there a reason for that? sorry maybe i am missing something! Screenshot 2022-12-05 at 09 18 08

thanks for finding these, removed the message and added href prop

JohnTParsons
JohnTParsons previously approved these changes Dec 7, 2022
@baburay23 baburay23 self-requested a review December 8, 2022 13:20
@mutebg mutebg removed the ready for design review Please assist in getting this reviewed label Dec 12, 2022
@mutebg mutebg requested a review from JohnTParsons December 13, 2022 03:29
@mutebg mutebg merged commit a848956 into main Dec 14, 2022
@mutebg mutebg deleted the feat/PPDSC-2609-submenu-overrides branch December 14, 2022 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This change contains a new feature ready for review Please assist in getting this reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants