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

LPS-168771 - Migrate Role Permission Navigation to Clay Vertical Nav #1108

Closed
wants to merge 10 commits into from
Closed

LPS-168771 - Migrate Role Permission Navigation to Clay Vertical Nav #1108

wants to merge 10 commits into from

Conversation

ethib137
Copy link

@ethib137 ethib137 commented May 9, 2023 •
edited
Loading

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

CI is automatically triggering the following test suites:

  •     ci:test:relevant
  •     ci:test:security
  •     ci:test:sf

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:sf - 1 out of 1 jobs passed in 5 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 56ea09f602ee231f3292ead33b58caf70c02a67d

Sender Branch:

Branch Name: LPS-168771-commits
Branch GIT ID: 9c943d0bb9e1eb27dc0bc63d1183fe0ec71b3ac0

1 out of 1jobs PASSED
1 Successful Jobs:
For more details click here.

@liferay-continuous-integration
Copy link
Collaborator

@liferay-continuous-integration
Copy link
Collaborator

❌ ci:test:security - 280 out of 288 jobs passed in 1 hour 34 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 56ea09f602ee231f3292ead33b58caf70c02a67d

Upstream Comparison:

Branch GIT ID: 56ea09f602ee231f3292ead33b58caf70c02a67d
Jenkins Build URL: EE Development Acceptance (master) - 4522 - 2023-05-09[08:36:38]

ci:test:security - 280 out of 288 jobs PASSED

8 Failed Jobs:

280 Successful Jobs:
    For more details click here.

    Failures unique to this pull:

    6. ...

    @liferay-continuous-integration
    Copy link
    Collaborator

    @liferay-continuous-integration
    Copy link
    Collaborator

    ❌ ci:test:stable - 27 out of 28 jobs passed

    ❌ ci:test:relevant - 74 out of 77 jobs passed in 1 hour 48 minutes

    Click here for more details.

    Base Branch:

    Branch Name: master
    Branch GIT ID: 56ea09f602ee231f3292ead33b58caf70c02a67d

    Upstream Comparison:

    Branch GIT ID: 56ea09f602ee231f3292ead33b58caf70c02a67d
    Jenkins Build URL: EE Development Acceptance (master) - 4522 - 2023-05-09[08:36:38]

    ci:test:stable - 27 out of 28 jobs PASSED

    1 Failed Jobs:

    27 Successful Jobs:
      ci:test:relevant - 74 out of 77 jobs PASSED

      3 Failed Jobs:

      74 Successful Jobs:
        For more details click here.

        Failures unique to this pull:

        1. modules-integration-mysql57-jdk8_stable/0/2
               [exec] * What went wrong:
     [exec] Execution failed for task ':baseline'.
     [exec] > Could not resolve all files for configuration ':baseline'.
     [exec]    > Could not resolve com.liferay.portal:com.liferay.portal.test:(,16.0.0).
     [exec]      Required by:
     [exec]          project :
     [exec]       > Failed to list versions for com.liferay.portal:com.liferay.portal.test.
     [exec]          > Unable to load Maven meta-data from https://repository-cdn.liferay.com/nexus/content/groups/public/com/liferay/portal/com.liferay.portal.test/maven-metadata.xml.
     [exec]             > Could not get resource 'https://repository-cdn.liferay.com/nexus/content/groups/public/com/liferay/portal/com.liferay.portal.test/maven-metadata.xml'.
     [exec]                > Could not GET 'https://repository-cdn.liferay.com/nexus/content/groups/public/com/liferay/portal/com.liferay.portal.test/maven-metadata.xml'.
     [exec]                   > repository-cdn.liferay.com: Name or service not known
     [exec] 
     [exec] * Try:
     [exec] Run with --info or --debug option to get more log output. Run with --scan to get full insights.
     [exec] 
     [exec] * Exception is:
     [exec] org.gradle.api.tasks.TaskExecutionException: Execution failed for task ':baseline'.
     [exec]   at org.gradle.api.internal.tasks.execution.CatchExceptionTaskExecuter.execute(CatchExceptionTaskExecuter.java:38)
     [exec]   at org.gradle.api.internal.tasks.execution.EventFiringTaskExecuter\$1.executeTask(EventFiringTaskExecuter.java:77)
     [exec]   at org.gradle.api.internal.tasks.execution.EventFiringTaskExecuter\$1.call(EventFiringTaskExecuter.java:55)
     [exec]   at org.gradle.api.internal.tasks.execution.EventFiringTaskExecuter\$1.call(EventFiringTaskExecuter.java:52)
     [exec]   at org.gradle.internal.operations.DefaultBuildOperationRunner\$CallableBuildOperationWorker.execute(DefaultBuildOperationRunner.java:200)
     [exec]   at org.gradle.internal.operations.DefaultBuildOperationRunner\$CallableBuildOperationWorker.execute(DefaultBuildOperationRunner.java:195)
     [exec]   at org.gradle.internal.operations.DefaultBuildOperationRunner\$3.execute(DefaultBuildOperationRunner.java:75)
     [exec]   at org.gradle.internal.operations.DefaultBuildOperationRunner\$3.execute(DefaultBuildOperationRunner.java:68)
     [exec]   at org.gradle.internal.operations.DefaultBuildOperationRunner.execute(DefaultBuildOperationRunner.java:153)
          1. com.liferay.jenkins.Jenkins
          2. com.liferay.portal.dao.orm.test.DefaultActionableDynamicQueryTest.testOrderByDescending - UNTESTED
          3. com.liferay.portal.dao.orm.test.SQLEscapedConcatTest.testConcatWithEscapedQuotes - UNTESTED
          4. ...

        For upstream results, click here.

        @liferay-continuous-integration
        Copy link
        Collaborator

        @ethib137 ethib137 mentioned this pull request May 10, 2023
        Closed
        @ethib137
        Copy link
        Author

        Hey @john-co at least one test is failing because the test is trying to find an element based on an ID that I removed. Many of the ids and classes are no longer needed since we are using the Clay component, so I don't want to add them there unnecessarily. Can you let me know what elements we would need QA identifiers for, and I can add in data-qa-ids instead?

        @john-co
        Copy link

        john-co commented May 10, 2023

        ci:test:bundle

        @liferay-continuous-integration
        Copy link
        Collaborator

        ✔️ ci:test:bundle - 1 out of 1 jobs passed in 29 minutes

        Click here for more details.

        Base Branch:

        Branch Name: master
        Branch GIT ID: 73ab9bf82e0d38f99fabc6a51416cfe7bc2f07d1

        Upstream Comparison:

        Branch GIT ID: 73ab9bf82e0d38f99fabc6a51416cfe7bc2f07d1
        Jenkins Build URL: EE Development Acceptance (master) - 4526 - 2023-05-10[08:38:17]

        ci:test:bundle - 1 out of 1 jobs PASSED
        For more details click here.
        Test bundle downloads:

        @liferay-continuous-integration
        Copy link
        Collaborator

        @john-co
        Copy link

        john-co commented May 10, 2023

        ci:test:roles

        @john-co
        Copy link

        john-co commented May 10, 2023

        Can you let me know what elements we would need QA identifiers for, and I can add in data-qa-ids instead?

        Thanks @ethib137 , for consistency with existing side panel at Widget Pages, at the element with @class=lfr-tooltip-scope that is just above Search and menubar-vertical-expand-lg, I'd suggest adding something like @data-qa-id=verticalNavMenu. Or if we are ok with identifying clay in the attribute, maybe something like clayVerticalNavMenu.
        screenshot_67

        Comparison with the addPanelBody for Widget Pages
        screenshot_66

        @ethib137
        Copy link
        Author

        ethib137 commented Jun 6, 2023

        Hi @stian-sigvartsen that makes sense. Thanks for your help.

        drewbrokke
        drewbrokke approved these changes Jun 7, 2023
        View reviewed changes
        Copy link

        @drewbrokke drewbrokke left a comment

        Choose a reason for hiding this comment

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

        Sorry about the delay on this one @ethib137. Looks good on my end, thanks!

        @stian-sigvartsen
        Copy link
        Collaborator

        @ethib137 looks good to me also! I will forward. Thanks!

        @stian-sigvartsen
        Copy link
        Collaborator

        ci:forward:force

        @liferay-continuous-integration
        Copy link
        Collaborator

        CI is automatically triggering the following test suites:

        •     ci:test:relevant
        •     ci:test:sf
        •     ci:test:stable

        The pull request will automatically be forwarded to the user brianchandotcom if the following test suites complete:

        •     ci:test:relevant
        •     ci:test:sf
          AND If the following test suites pass:
        •     ci:test:stable

        @liferay-continuous-integration
        Copy link
        Collaborator

        ✔️ ci:test:sf - 1 out of 1 jobs passed in 5 minutes

        Click here for more details.

        Base Branch:

        Branch Name: master
        Branch GIT ID: 629e14a3812095eb35ed50cb33263e621f225ccc

        Sender Branch:

        Branch Name: LPS-168771-commits
        Branch GIT ID: 489f6d9224ef6b40ecc015f91eec29ebdd172f1b

        1 out of 1jobs PASSED
        1 Successful Jobs:
        For more details click here.

        @liferay-continuous-integration
        Copy link
        Collaborator

        @liferay-continuous-integration
        Copy link
        Collaborator

        ✔️ ci:test:stable - 29 out of 29 jobs passed

        ✔️ ci:test:relevant - 80 out of 81 jobs passed in 2 hours 19 minutes

        Click here for more details.

        Base Branch:

        Branch Name: master
        Branch GIT ID: 629e14a3812095eb35ed50cb33263e621f225ccc

        Upstream Comparison:

        Branch GIT ID: 629e14a3812095eb35ed50cb33263e621f225ccc
        Jenkins Build URL: EE Development Acceptance (master) - 4609 - 2023-06-13[08:38:53]

        ci:test:stable - 29 out of 29 jobs PASSED
        29 Successful Jobs:
          ci:test:relevant - 79 out of 81 jobs PASSED

          2 Failed Jobs:

          79 Successful Jobs:
            For more details click here.

            This pull contains no unique failures.

            Failures in common with acceptance upstream results at 629e14a:
            1. unit-jdk8/0/0
              1. com.liferay.taglib.util.HtmlTopTagTest.testDataSennaTrackAttribute - UNTESTED

            @liferay-continuous-integration
            Copy link
            Collaborator

            All required test suite(s) passed.
            Forwarding pull request to brianchandotcom.
            Console

            @liferay-continuous-integration
            Copy link
            Collaborator

            Error has occurred while attempting to forward pull request to brianchandotcom. Retrying in 1 minute...
            See console log for detail:Full Console

            1 similar comment
            @liferay-continuous-integration
            Copy link
            Collaborator

            Error has occurred while attempting to forward pull request to brianchandotcom. Retrying in 1 minute...
            See console log for detail:Full Console

            @liferay-continuous-integration
            Copy link
            Collaborator

            Error has occurred while forwarding pull request to brianchandotcom.
            Please try again later or contact the CI team for assistance.
            See console log for details: Full Console

            @liferay-continuous-integration
            Copy link
            Collaborator

            @ethib137
            Copy link
            Author

            Hi @noraszel, I don't see any conflict. Do you mean that I should manually forward the PR to BChan?

            @noraszel
            Copy link
            Collaborator

            noraszel commented Jun 14, 2023 •
            edited
            Loading

            Hi @ethib137, I commented because of the conflict in the
            modules/apps/roles/roles-admin-web/src/main/resources/META-INF/resources/init.jsp file. But @stian-sigvartsen let me know he is already working on this. So I deleted my comment, sorry.

            @stian-sigvartsen stian-sigvartsen mentioned this pull request Jun 14, 2023
            Closed
            @stian-sigvartsen
            Copy link
            Collaborator

            Manually forwarded to brianchandotcom/pull/136336

            Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
            Reviewers

            @matuzalemsteles matuzalemsteles matuzalemsteles left review comments

            @drewbrokke drewbrokke drewbrokke approved these changes

            Assignees
            No one assigned
            Labels
            ci:test:bundle - success ci:test:relevant - success ci:test:roles - failure ci:test:security - failure ci:test:sf - success ci:test:stable - success
            Projects
            None yet
            Milestone
            No milestone
            Development

            Successfully merging this pull request may close these issues.

            Vertical Nav overlaps items with long nested list
            7 participants
            @ethib137 @liferay-continuous-integration @john-co @stian-sigvartsen @noraszel @drewbrokke @matuzalemsteles