Fix two active navigation pills #524
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a UI bug where it displayed two navigation pills as active instead of one. For example when clicking on `How it works` link in tbtc both `Bridge` and `How it works` links were active. We've fixed that by adding a new method - `addActiveStatusToPills` - which adds `isActive` property to each of the link. If there are two or more links that are active (based on `useMatch` hook) then we only keep the active status for the last one.
|
Preview uploaded to https://preview.dashboard.test.threshold.network/subnav-pills-fix/index.html. |
r-czajkowski
requested changes
May 30, 2023
|
Preview uploaded to https://preview.dashboard.test.threshold.network/subnav-pills-fix/index.html. |
The previous fix used hooks inside loops which should not be done (see https://legacy.reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level). As a workaround I decided to refactor the fix. Now we gather all resolved paths for all the pills that are in the menu using `resolvePath` function and we pass those paths to each pill component. Inside the component we check if `useMatch` hook returns anything for the given pill and if yes then we check if is is because it has asterisk (`*`) in it's path. If yes, then we first have to check if the url match any of the other pills using our array of resolved paths. If not then we can safely assume that this pill can be active.
We are not inside any loop so we can safely use `useMatch` hook.
|
Preview uploaded to https://preview.dashboard.test.threshold.network/subnav-pills-fix/index.html. |
r-czajkowski
requested changes
Jul 17, 2023
I've manage to resolve the issue using `resolvePath` and `matchPath` functions instead of `useMatch` and `useResolvedPath` hooks. For that I had to introduce a new property - `parentPath` - that is passed to `SubNavigationPills` component and will be used as a `pathnaemBase` for the `resolvePath` function. If more than one pill is active then the one with the longest `pathnameBase` from the `matchPath` function will be active. This will happen when there are two paths with asterisk, for example: `tBTC/*`, `tBTC/how-it-work/*`. For the path `tBTC/how-it-work/overview` we want only the second one to be active so we are picking the one with the longest `pathnameBase`. In the first case the pathnameBase is `tBTC` and in the second one it's `tBTC/how-it-works`.
@r-czajkowski It should be fixed now in 780e859 |
|
Preview uploaded to https://preview.dashboard.test.threshold.network/subnav-pills-fix/index.html. |
|
Preview uploaded to https://preview.dashboard.test.threshold.network/subnav-pills-fix/index.html. |
The navigation did not work in preview links because the branch name was in the url. This resulted in `window.location.pathname` returning wrong pathname.
michalsmiarowski
force-pushed
the
subnav-pills-fix
branch
from
b6cd43b
to
3113a7e
Compare
|
Preview uploaded to https://preview.dashboard.test.threshold.network/subnav-pills-fix/index.html. |
|
Preview uploaded to https://preview.dashboard.test.threshold.network/subnav-pills-fix/index.html. |
michalsmiarowski
force-pushed
the
subnav-pills-fix
branch
from
cc96c36
to
dee2428
Compare
|
Preview uploaded to https://preview.dashboard.test.threshold.network/subnav-pills-fix/index.html. |
Having `parentPathBase` in `ParentComponent` type (but not in `RouteProps`) actually complicated more thing than it resolves. I've decided to put it in `RouteProps` as an optional parameter. Then we assing the correct `parentPathBase` to a specific `RouteProps` object inside `renderPageComponent` function. This way we don't have to pass `parentPathBase` parameter to all PageComponents manually all the way to `SubNavigaionPillsProps` because we already have it in `links` parameter.
|
Preview uploaded to https://preview.dashboard.test.threshold.network/subnav-pills-fix/index.html. |
r-czajkowski
approved these changes
Aug 16, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #501
There was a UI bug where it displayed two navigation pills as active instead of one. For example when clicking on
How it workslink in tbtc bothBridgeandHow it workslinks were active.We've fixed that by adding a new method -
addActiveStatusToPills- which addsisActiveproperty to each of the link. If there are two or more links that are active (based onuseMatchhook) then we only keep the active status for the last one.Additionally I've also fixed the sub-navigation for
Stakingpage because theStakingpill was not active when being on authorization page.Screenshot of the fix