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

LPS-168771 - Migrate Role Permission Navigation to Clay Vertical Nav (rebased) #136336

Closed

Conversation

stian-sigvartsen
Copy link

@stian-sigvartsen stian-sigvartsen commented Jun 14, 2023

Manually forwarding because ci:forward fails on liferay-appsec/pull/1108


https://issues.liferay.com/browse/LPS-168771

This PR seeks to solve accessibility issues that relate to the Role Permissions Navigation. Initially we wanted to only update the markup in the jsp, but this resulted in more complexity trying to keep the javascript working that depended on the old markup. As a result it made more sense to move towards the best option of using the Clay Vertical Nav component directly.

What has changed:

  • Due to it being React, there is a slight lag and the navigation loads slightly after the page.
  • The old behavior had all panels open when filtering and all panels close when the filter input is emptied. This is currently not possible with the Clay component. All panels now open when filtering and when done filtering it reverts back to the initial state when loaded. This makes more sense to me than closing all the panels after filtering.
  • There is an existing bug with the Clay Vertical Nav that causes some children to overlap when multiple levels deep. This will be fixed here. Fixed
  • At some point the persistence after refresh of the currently selected item was lost. This PR adds that functionality back.

Since this is a larger rewrite I'm asking for additional testing by @john-co to make sure we don't have any regressions.

@liferay-continuous-integration
Copy link
Collaborator

Closing pull request because all liferay-portal pullrequests sent to Brian Chan must be sent by using ci:forward on a pull request that was sent to someone else.

@stian-sigvartsen
Copy link
Author

ci:reopen

@brianchandotcom
Copy link
Owner

@ethib137 I haven't seen this pattern before?

modules/apps/roles/roles-admin-web/src/main/java/com/liferay/roles/admin/web/internal/display/context/ClayVerticalNavItem.java

Can you resend without the new pattern? Thx.

@stian-sigvartsen fyi

@brianchandotcom
Copy link
Owner

git grep JsonInclude **Clay**.java

returns nothing

git ls-files **/display/context/**Clay**.java

only returns samples

@ethib137 @stian-sigvartsen @drewbrokke fyi

@brianchandotcom
Copy link
Owner

@ethib137 @drewbrokke can you guys resend with the map instead?

The map is more verbose, but less complicated in the long run. And we use the map pattern in lots of other places.

@ethib137
Copy link

Hey @brianchandotcom , sure no problem. Just to clarify, do you mean we should revert this commit so that all the navItems are created directly as JSONArrays and JSONObjects, or do you mean that the ClayVerticalNavItem class should be refactored to remove all the Jackson bindings?

@ethib137
Copy link

Resent with updates here: liferay-appsec#1179

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vertical Nav overlaps items with long nested list
5 participants