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-2523): sub menu component #464

Merged
merged 20 commits into from
Nov 11, 2022
Merged

Conversation

Xin00163
Copy link
Contributor

@Xin00163 Xin00163 commented Nov 8, 2022

PPDSC-2523

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 Nov 8, 2022
@newskit-engineering
Copy link
Collaborator

@Xin00163 Xin00163 added the ready for design review Please assist in getting this reviewed label Nov 8, 2022
@Xin00163 Xin00163 added ready for review Please assist in getting this reviewed and removed ready for design review Please assist in getting this reviewed labels Nov 9, 2022
src/menu/__tests__/menu.stories.tsx Outdated Show resolved Hide resolved
src/menu/__tests__/menu.stories.tsx Show resolved Hide resolved
src/menu/styled.tsx Outdated Show resolved Hide resolved
@Vanals
Copy link
Contributor

Vanals commented Nov 10, 2022

I think the feedbacks we give when we open sub components is very hard to see or realise, as we do not have any "selected" style colour. I think we should have some, I don't think the arrow changing direction is enough, everything looks a bit the same, especially in the "horizontal" example, is a bit confusing where I am

Screenshot 2022-11-10 at 14 14 49

@Vanals
Copy link
Contributor

Vanals commented Nov 10, 2022

Also, do you mind ticking the other check boxes? I think being a new component might be important to double check Screen readers behaves good

@Xin00163
Copy link
Contributor Author

Xin00163 commented Nov 10, 2022

I think the feedbacks we give when we open sub components is very hard to see or realise, as we do not have any "selected" style colour. I think we should have some, I don't think the arrow changing direction is enough, everything looks a bit the same, especially in the "horizontal" example, is a bit confusing where I am

Screenshot 2022-11-10 at 14 14 49

That's the default stylePreset for menuItem horizontal. I could apply the menuVertical overrides stylePreset to it in the storybook scenario. Would that be clear?

@Vanals
Copy link
Contributor

Vanals commented Nov 10, 2022

I think the feedbacks we give when we open sub components is very hard to see or realise, as we do not have any "selected" style colour. I think we should have some, I don't think the arrow changing direction is enough, everything looks a bit the same, especially in the "horizontal" example, is a bit confusing where I am
Screenshot 2022-11-10 at 14 14 49

That's the default stylePreset for menu horizontal. I could apply the menuVertical overrides stylePreset to it in the storybook scenario. Would that be clear?

If this is the default, my point is, by default it should clearly communicate to the user where they are in the three. I think by default is confusing and by default should be not. No overrides should be needed to make it "work" properly and easy to use IMO. If make sense I would get back to designers and see

@Xin00163
Copy link
Contributor Author

Xin00163 commented Nov 10, 2022

I think the feedbacks we give when we open sub components is very hard to see or realise, as we do not have any "selected" style colour. I think we should have some, I don't think the arrow changing direction is enough, everything looks a bit the same, especially in the "horizontal" example, is a bit confusing where I am
Screenshot 2022-11-10 at 14 14 49

That's the default stylePreset for menu horizontal. I could apply the menuVertical overrides stylePreset to it in the storybook scenario. Would that be clear?

If this is the default, my point is, by default it should clearly communicate to the user where they are in the three. I think by default is confusing and by default should be not. No overrides should be needed to make it "work" properly and easy to use IMO. If make sense I would get back to designers and see

̶O̶K̶.̶ ̶I̶ ̶w̶i̶l̶l̶ ̶b̶r̶i̶n̶g̶ ̶i̶t̶ ̶u̶p̶ ̶w̶i̶t̶h̶ ̶M̶i̶k̶e̶.̶
̶@̶V̶a̶n̶a̶l̶s̶ ̶N̶o̶w̶ ̶t̶h̶a̶t̶ ̶M̶e̶n̶u̶ ̶i̶s̶ ̶u̶s̶e̶d̶ ̶b̶y̶ ̶o̶t̶h̶e̶r̶ ̶t̶e̶a̶m̶s̶,̶ ̶w̶e̶ ̶c̶a̶n̶'̶t̶ ̶c̶h̶a̶n̶g̶e̶ ̶m̶e̶n̶u̶I̶t̶e̶m̶ ̶s̶t̶y̶l̶e̶P̶r̶e̶s̶e̶t̶ ̶w̶i̶t̶h̶o̶u̶t̶ ̶m̶a̶k̶i̶n̶g̶ ̶a̶ ̶b̶r̶e̶a̶k̶i̶n̶g̶ ̶c̶h̶a̶n̶g̶e̶.̶ ̶A̶s̶ ̶m̶e̶n̶u̶S̶u̶b̶ ̶i̶s̶ ̶s̶u̶p̶p̶o̶s̶e̶d̶ ̶t̶o̶ ̶b̶e̶ ̶c̶o̶n̶s̶i̶s̶t̶e̶n̶t̶ ̶w̶i̶t̶h̶ ̶m̶e̶n̶u̶I̶t̶e̶m̶,̶ ̶w̶e̶ ̶a̶r̶e̶ ̶u̶s̶i̶n̶g̶ ̶t̶h̶e̶ ̶s̶a̶m̶e̶ ̶s̶t̶y̶l̶e̶P̶r̶e̶s̶e̶t̶.̶ ̶A̶f̶t̶e̶r̶ ̶c̶h̶e̶c̶k̶i̶n̶g̶ ̶w̶i̶t̶h̶ ̶M̶i̶k̶e̶,̶ ̶w̶e̶ ̶a̶r̶e̶ ̶m̶a̶k̶i̶n̶g̶ ̶a̶ ̶s̶m̶a̶l̶l̶ ̶c̶h̶a̶n̶g̶e̶ ̶i̶n̶ ̶s̶e̶l̶e̶c̶t̶e̶d̶ ̶f̶o̶r̶ ̶m̶e̶n̶u̶S̶u̶b̶ ̶s̶t̶y̶l̶e̶P̶r̶e̶s̶e̶t̶.̶ ̶T̶h̶e̶ ̶t̶e̶x̶t̶ ̶c̶o̶l̶o̶r̶ ̶i̶s̶ ̶n̶o̶w̶ ̶̶i̶n̶k̶C̶o̶n̶t̶r̶a̶s̶t̶̶ ̶t̶o̶ ̶i̶n̶d̶i̶c̶a̶t̶e̶ ̶i̶t̶'̶s̶ ̶s̶e̶l̶e̶c̶t̶e̶d̶.̶

@mutebg just reminded me that we have a selected prop so user can pass to indicate it's selected

mutebg
mutebg previously approved these changes Nov 11, 2022
@Xin00163 Xin00163 merged commit f372164 into main Nov 11, 2022
@Xin00163 Xin00163 deleted the feat/PPDSC-2523-sub-menu-component branch November 11, 2022 16:17
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.

5 participants