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

[Fix] Inactive State on 'Discover' Tab in Side Navigation Menu #5432

Merged
merged 13 commits into from
Dec 13, 2023
Merged

[Fix] Inactive State on 'Discover' Tab in Side Navigation Menu #5432

merged 13 commits into from
Dec 13, 2023

Conversation

willie-hung
Copy link
Contributor

@willie-hung willie-hung commented Nov 5, 2023 •
edited
Loading

Description

This PR resolves the issue where the 'Discover' tab in the collapsible side navigation menu failed to display the appropriate active state styling upon selection. The modification ensures that when a user selects the 'Discover' tab, it is now triggered and displayed with an active state background.

Issues Resolved

Resolves #5226

Screenshot

Before

before.mov

After

after.mov

Testing

Add unit tests for the following functions to ensure the newly added function operates correctly.

  • isActiveNavLink (newly added function)
  • createEuiListItem

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@willie-hung
Update CHANGELOG.md
0378e8b 
Signed-off-by: Willie Hung <[email protected]>
@willie-hung willie-hung changed the title [Fix] Fix Inactive State on 'Discover' Tab in Side Navigation Menu [Fix] Inactive State on 'Discover' Tab in Side Navigation Menu Nov 5, 2023
@codecov Codecov
Copy link

codecov bot commented Nov 5, 2023 •
edited
Loading

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (5fabb73) 66.97% compared to head (a37d42c) 66.97%.

Files Patch % Lines
src/core/public/chrome/ui/header/nav_link.tsx 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5432      +/-   ##
==========================================
- Coverage   66.97%   66.97%   -0.01%     
==========================================
  Files        3293     3293              
  Lines       63289    63292       +3     
  Branches    10065    10066       +1     
==========================================
  Hits        42390    42390              
- Misses      18458    18459       +1     
- Partials     2441     2443       +2
Flag Coverage Δ
Linux_1 35.24% <66.66%> (+<0.01%) ⬆️
Linux_2 55.17% <66.66%> (+<0.01%) ⬆️
Linux_3 43.80% <66.66%> (-0.01%) ⬇️
Linux_4 35.34% <66.66%> (+<0.01%) ⬆️
Windows_1 35.27% <66.66%> (+<0.01%) ⬆️
Windows_2 55.14% <66.66%> (+<0.01%) ⬆️
Windows_3 43.80% <66.66%> (+<0.01%) ⬆️
Windows_4 35.34% <66.66%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

willie-hung and others added 3 commits November 5, 2023 13:35
@willie-hung
Revise test suit
e69ff56 
Signed-off-by: Willie Hung <[email protected]>
@willie-hung
Add isActive logic
4cd1632 
Signed-off-by: Willie Hung <[email protected]>
@joshuarrrr
Merge branch 'main' into bug/discover-tab
8f1bedf 
Signed-off-by: Josh Romero <[email protected]>
ananzh
ananzh previously approved these changes Nov 13, 2023
View reviewed changes
@ananzh ananzh added bug Something isn't working v2.12.0 backport 2.x labels Nov 13, 2023
joshuarrrr
joshuarrrr reviewed Nov 13, 2023
View reviewed changes
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

@Willie-The-Lord I think this PR is great, but I have a couple ideas for how we might make it a little better oriented with future improvements.

src/core/public/chrome/ui/header/nav_link.tsx Outdated
@@ -39,6 +39,15 @@ import { relativeToAbsolute } from '../../nav_links/to_nav_link';
export const isModifiedOrPrevented = (event: React.MouseEvent<HTMLButtonElement, MouseEvent>) =>
event.metaKey || event.altKey || event.ctrlKey || event.shiftKey || event.defaultPrevented;

// Future: should make this function more generic
Copy link
Member

Choose a reason for hiding this comment

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

For comments that indicate future work, we generally use the TODO: prefix, which is actually parsed and identified by several tools. Also, in the case of any TODO, there should be an open issue, and the link should be included in the comment itself. This makes it much easier to know if a given todo has actually been completed or if the comment is still valid.

Suggested change
// Future: should make this function more generic
// TODO: make this function more generic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshuarrrr should I create an issue for the future improvement?

Copy link
Member

Choose a reason for hiding this comment

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

@Willie-The-Lord Yes, please, that would be appreciated! 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshuarrrr issue created.

src/core/public/chrome/ui/header/nav_link.tsx Outdated Show resolved Hide resolved
src/core/public/chrome/ui/header/nav_link.test.tsx Outdated Show resolved Hide resolved
@willie-hung @joshuarrrr
Update src/core/public/chrome/ui/header/nav_link.tsx
6dae6d1 
Co-authored-by: Josh Romero <[email protected]>
Signed-off-by: Willie Hung <[email protected]>
ananzh
ananzh previously approved these changes Dec 12, 2023
View reviewed changes
@willie-hung @SuZhou-Joe
Update src/core/public/chrome/ui/header/nav_link.tsx
a37d42c 
Co-authored-by: SuZhou-Joe <[email protected]>
Signed-off-by: Willie Hung <[email protected]>
joshuarrrr
joshuarrrr approved these changes Dec 13, 2023
View reviewed changes
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

LGTM - @Willie-The-Lord please open a follow-up issue, if you haven't already, for creating a registration function.

@ananzh ananzh merged commit 478176a into opensearch-project:main Dec 13, 2023
57 of 58 checks passed
@willie-hung willie-hung deleted the bug/discover-tab branch December 13, 2023 20:25
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Dec 13, 2023
Merged
opensearch-trigger-bot bot pushed a commit that referenced this pull request Dec 13, 2023
@github-actions @joshuarrrr @SuZhou-Joe
[Fix] Inactive State on 'Discover' Tab in Side Navigation Menu (#5432)
30d675f 
* Fix active background render error
* Update CHANGELOG.md
* Revise test suit
* Add isActive logic
* Remove redundant tests and add comment for future reference
* Update src/core/public/chrome/ui/header/nav_link.tsx
* Seperate special cases from logic && revise test
* Update CHANGELOG.md
* Update src/core/public/chrome/ui/header/nav_link.tsx

Co-authored-by: SuZhou-Joe <[email protected]>
Signed-off-by: Willie Hung <[email protected]>

---------

Signed-off-by: Willie Hung <[email protected]>
Signed-off-by: Josh Romero <[email protected]>
Co-authored-by: Josh Romero <[email protected]>
Co-authored-by: SuZhou-Joe <[email protected]>
(cherry picked from commit 478176a)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@willie-hung willie-hung mentioned this pull request Dec 13, 2023
Open
abbyhu2000 pushed a commit that referenced this pull request Dec 13, 2023
@opensearch-trigger-bot @github-actions
@joshuarrrr @SuZhou-Joe
[Fix] Inactive State on 'Discover' Tab in Side Navigation Menu (#5432) (
953fb81 
#5606)

* Fix active background render error
* Update CHANGELOG.md
* Revise test suit
* Add isActive logic
* Remove redundant tests and add comment for future reference
* Update src/core/public/chrome/ui/header/nav_link.tsx
* Seperate special cases from logic && revise test
* Update CHANGELOG.md
* Update src/core/public/chrome/ui/header/nav_link.tsx




---------





(cherry picked from commit 478176a)

Signed-off-by: Willie Hung <[email protected]>
Signed-off-by: Josh Romero <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Josh Romero <[email protected]>
Co-authored-by: SuZhou-Joe <[email protected]>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 26, 2024
@github-actions @joshuarrrr @SuZhou-Joe
[Fix] Inactive State on 'Discover' Tab in Side Navigation Menu (#5432)
fa4945c 
* Fix active background render error
* Update CHANGELOG.md
* Revise test suit
* Add isActive logic
* Remove redundant tests and add comment for future reference
* Update src/core/public/chrome/ui/header/nav_link.tsx
* Seperate special cases from logic && revise test
* Update CHANGELOG.md
* Update src/core/public/chrome/ui/header/nav_link.tsx

Co-authored-by: SuZhou-Joe <[email protected]>
Signed-off-by: Willie Hung <[email protected]>

---------

Signed-off-by: Willie Hung <[email protected]>
Signed-off-by: Josh Romero <[email protected]>
Co-authored-by: Josh Romero <[email protected]>
Co-authored-by: SuZhou-Joe <[email protected]>
(cherry picked from commit 478176a)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Jan 26, 2024
Merged
manasvinibs pushed a commit that referenced this pull request Jan 31, 2024
@github-actions @joshuarrrr
@SuZhou-Joe @manasvinibs
[Fix] Inactive State on 'Discover' Tab in Side Navigation Menu (#5432)
e3f5077 
* Fix active background render error
* Update CHANGELOG.md
* Revise test suit
* Add isActive logic
* Remove redundant tests and add comment for future reference
* Update src/core/public/chrome/ui/header/nav_link.tsx
* Seperate special cases from logic && revise test
* Update CHANGELOG.md
* Update src/core/public/chrome/ui/header/nav_link.tsx

Co-authored-by: SuZhou-Joe <[email protected]>
Signed-off-by: Willie Hung <[email protected]>

---------

Signed-off-by: Willie Hung <[email protected]>
Signed-off-by: Josh Romero <[email protected]>
Co-authored-by: Josh Romero <[email protected]>
Co-authored-by: SuZhou-Joe <[email protected]>
(cherry picked from commit 478176a)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
manasvinibs pushed a commit that referenced this pull request Jan 31, 2024
@opensearch-trigger-bot @github-actions
@joshuarrrr @SuZhou-Joe
[Fix] Inactive State on 'Discover' Tab in Side Navigation Menu (#5432) (
4757671 
#5744)

* Fix active background render error
* Update CHANGELOG.md
* Revise test suit
* Add isActive logic
* Remove redundant tests and add comment for future reference
* Update src/core/public/chrome/ui/header/nav_link.tsx
* Seperate special cases from logic && revise test
* Update CHANGELOG.md
* Update src/core/public/chrome/ui/header/nav_link.tsx




---------





(cherry picked from commit 478176a)

Signed-off-by: Willie Hung <[email protected]>
Signed-off-by: Josh Romero <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Josh Romero <[email protected]>
Co-authored-by: SuZhou-Joe <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuZhou-Joe SuZhou-Joe SuZhou-Joe left review comments

@ananzh ananzh ananzh approved these changes

@joshuarrrr joshuarrrr joshuarrrr approved these changes

@kavilla kavilla Awaiting requested review from kavilla kavilla is a code owner

@seanneumann seanneumann Awaiting requested review from seanneumann

@AMoo-Miki AMoo-Miki Awaiting requested review from AMoo-Miki AMoo-Miki is a code owner

@ashwin-pc ashwin-pc Awaiting requested review from ashwin-pc ashwin-pc is a code owner

@abbyhu2000 abbyhu2000 Awaiting requested review from abbyhu2000 abbyhu2000 is a code owner

@zengyan-amazon zengyan-amazon Awaiting requested review from zengyan-amazon zengyan-amazon is a code owner

@kristenTian kristenTian Awaiting requested review from kristenTian

@zhongnansu zhongnansu Awaiting requested review from zhongnansu zhongnansu is a code owner

@manasvinibs manasvinibs Awaiting requested review from manasvinibs manasvinibs is a code owner

@ZilongX ZilongX Awaiting requested review from ZilongX ZilongX is a code owner

@Flyingliuhub Flyingliuhub Awaiting requested review from Flyingliuhub Flyingliuhub is a code owner

@BSFishy BSFishy Awaiting requested review from BSFishy

@curq curq Awaiting requested review from curq curq is a code owner

@bandinib-amzn bandinib-amzn Awaiting requested review from bandinib-amzn bandinib-amzn is a code owner

Assignees
No one assigned
Labels
backport 2.x bug Something isn't working repeat-contributor v2.12.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] slide drawer navigation item 'Discover' doesn't have active background when selected
4 participants
@willie-hung @joshuarrrr @SuZhou-Joe @ananzh