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.

Sign up for GitHub

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

Jump to bottom

[Feature Request]: Update HeaderMenuItem to use isActive, like SideNavMenuItem #13369

Closed
4 tasks
tay1orjones opened this issue Mar 17, 2023 · 3 comments · Fixed by #13375
Closed
4 tasks

[Feature Request]: Update HeaderMenuItem to use isActive, like SideNavMenuItem #13369

tay1orjones opened this issue Mar 17, 2023 · 3 comments · Fixed by #13375
Labels
good first issue 👋 Used by GitHub to elevate contribution opportunities hacktoberfest See https://hacktoberfest.com/ needs: community contribution Due to roadmap and resource availability, we are looking for outside contributions on this issue. proposal: accepted This request has gone through triaging and we are accepting PR's against it. type: enhancement 💡

Comments

@tay1orjones
Copy link
Member

tay1orjones commented Mar 17, 2023 •
edited
Loading

The problem

HeaderMenuItem uses isCurrentPage to trigger the "selected" state for the component, whereas in SideNavMenuItem the prop isActive is used. This is sort of confusing to have different named props doing functionally similar things, all within the UIShell.

The solution

Updating this component to use the same prop would be a nice unification of the component APIs.

Tasks
Beta Give feedback

Examples

https://ibm-studios.slack.com/archives/C2K6RFJ1G/p1679052333819769
@github-project-automation github-project-automation bot moved this to Triage in Roadmap Mar 17, 2023
@tay1orjones tay1orjones moved this from Triage to Ready for community implementation in Roadmap Mar 17, 2023
@tay1orjones tay1orjones added needs: community contribution Due to roadmap and resource availability, we are looking for outside contributions on this issue. good first issue 👋 Used by GitHub to elevate contribution opportunities hacktoberfest See https://hacktoberfest.com/ proposal: accepted This request has gone through triaging and we are accepting PR's against it. labels Mar 17, 2023
@pratikkarad
Copy link
Contributor

Hi @tay1orjones, I would like to work on this.

@pratikkarad
Copy link
Contributor

I took a look at the code and found out that the HeaderMenu component also uses the isCurrentPage to trigger the 'selected' state. I was wondering, shall I update this prop as well?

@pratikkarad pratikkarad mentioned this issue Mar 20, 2023
Merged
@tw15egan
Copy link
Collaborator

@pratikkarad yes, we should mirror these changes in both HeaderMenu and HeaderMenuItem. They can be bundled in the open PR

@kodiakhq kodiakhq bot closed this as completed in #13375 Mar 22, 2023
@github-project-automation github-project-automation bot moved this from Ready for community implementation to Completed in Roadmap Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue 👋 Used by GitHub to elevate contribution opportunities hacktoberfest See https://hacktoberfest.com/ needs: community contribution Due to roadmap and resource availability, we are looking for outside contributions on this issue. proposal: accepted This request has gone through triaging and we are accepting PR's against it. type: enhancement 💡
Projects
Roadmap
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(HeaderMenuItem): update prop pratikkarad/carbon
3 participants
@tay1orjones @tw15egan @pratikkarad