Conversation
Taking advantage of event bubbling[1] eliminates the need to pass a callback for every action rendered in the sidebar. Rather, the sidebar will close itself unless after clicking an action unless the it has the `data-keep-sidebar-open="true"` attribute. [1] https://developer.mozilla.org/en-US/docs/Learn/JavaScript/Building_blocks/Events#event_bubbling
Because it's useless from the testing point of view.
dgdavid
force-pushed
the
sidebar-improvements
branch
2 times, most recently
from
cf491db to
7260dfb
Compare
Because from the accessibility point of view link is the right element when navigating to somewhere. Buttons must be used for triggering actions that does not change browser focus or page location. See https://www.w3.org/TR/wai-aria-1.2/#link
By not displaying it when already in the product selection path.
This is yet another intermediate (and probably temporary) step toward defining the look & feel. Take it lightly. There is a lot of work to be done in that regard, but it will have to wait a while.
And start using it for grouping the Diagnostic tools in the Sidebar.
It's a combination of a teleported Layout slot and a component for allowing pages to put their relevant or related actions at the top of the Sidebar.
By closing it only if the click comes from an HTMLButtonElement or HTMLAnchorElement. Also, adapt it to look for the original target first to cover cases where we redispatched events using CustomEvents from Portals.
Now that pages can put their relevant actions at the top of the Sidebar, makes sense that the Overview page place the "Change selected product" there.
Now that the Network section summarizes the available connections, this component is not that relevant. It is being dropped to reduce the confusion of having two slightly different network summaries in two different locations.
The logic for not rendering it when already on the product selection page is no longer needed now that it's rendered on demand (when the user is on the Overview page at this moment).
That way, the use of React.useMemo to avoid re-rendering them in each visibility change is not longer needed. It simplifies things a lot.
dgdavid
force-pushed
the
sidebar-improvements
branch
from
091cf20 to
99df6db
Compare
By adding the product name to know in advance about what the information will be about.
dgdavid
commented
Mar 13, 2023
imobachgs
approved these changes
Mar 14, 2023
Contributor
imobachgs
left a comment
imobachgs
left a comment
There was a problem hiding this comment.
Awesome work! Just some tiny comments. Once they are addressed, you can merge.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Originally created for the iSCSI UI #435, this set of changes had the sole purpose of allowing us to teleport links from a page to a layout slot created for this use case. However, a few other changes were either necessary for the implementation of the feature or deemed important enough for the next release. Namely
Taken advantage of the event bubbling for auto-closing the sidebar when an action is clicked instead of passing the "closeCallback" around.
Dropped the
TargetIpsPopupaction and component since they became a bit useless now we have the addresses summary in the network section at Overview page.Adapted the action for navigating to the product selection page:
Use a link instead of a button, since it's an element that lets the user navigate
It's displayed only on the Overview page
Has been moved to the software namespace
Started using the dark green (aka Pine Green) as the primary color due to the low contrast of the lighter one (aka Jungle Green), which has been relegated to the :hover states (still having the same low contrast, though)
Kept betting on plain old HTML links. I.e., use the "default" underlined text for actions in the sidebar too (even though some of the actions there are actually buttons, but visually it is accepted to present them as links)
Use a custom disclosure widget for grouping some actions, like the "Diagnostic tools" ones.
The PatternFly ExpandableSection was discarded by now because it's not possible to add the custom
data-keep-sidebar-openattribute to the toggler.The native details/summary element was discarded because of https://www.scottohara.me/blog/2022/09/12/details-summary.html
Changed the test-suite a bit to use a MemoryRouter and improve how navigation is mocked. That was a change due to 0560498, which was later "reverted". Still, changes in test remain because it's cleaner now to deal with routes (see lines dropped at 0413912)
And maybe some other small stuff not summarized above.
Note: after latest changes, after screenshots are a bit out-dated. Now the link is "Change product" instead of "Change selected product"