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

[DEMO]: MAAS UI List Tree Fix #5336

Closed
wants to merge 38 commits into from
Closed

Conversation

jmuzina
Copy link
Member

@jmuzina jmuzina commented Sep 4, 2024

Done

This PR includes the changes from #5270, as well as updates to the custom styling from MAAS UI so that their .p-list-tree--static can be made compatible with our changes to the list tree.

This PR should NOT be merged into Vanilla - it is purely for demonstrative purposes for MAAS UI devs.

Note: this is based on #5270, so includes its changes. The interesting stuff is this:

  // MAAS UI static tree styling (`.p-list-tree--static`)
  .p-list-tree--static {
    // Force the entire tree to be visible
    display: block !important;

    // Always show the collapse button
    // This is only needed if you are running a script, like react components, that changes the `aria-hidden` of the `.p-list-tree`.
    .p-list-tree--static::before {
      @extend %list-tree-collapse-icon;
    }
    // Hide the default expand button
    .p-list-tree__toggle::before {
      display: none;
    }
  }

and the lack of toggling aria-hidden on .p-list-tree elements with JS for the maas tree example.

Screenshots

Screenshot 2024-09-04 at 6 26 00 PM

@webteam-app
Copy link

@bartaz bartaz changed the title DO NOT MERGE: MAAS UI List Tree Fix [DEMO]: MAAS UI List Tree Fix Sep 6, 2024
@bartaz
Copy link
Member

bartaz commented Oct 1, 2024

@jmuzina Is this still needed?

@jmuzina
Copy link
Member Author

jmuzina commented Oct 1, 2024

@jmuzina Is this still needed?

Yes, I haven't had the time yet to make this fix on maas-ui side and they are still on a rather old version of Vanilla, so it hasn't been a priority yet. Also, I've been having issues running maas-ui locally that I haven't resolved. But, it may be easier to get the ball rolling this way:

@ndv99 can you let me know what the priority is on updating maas UI to Vanilla 4.16.0, and try out the updates to .p-list-tree--static shown in this PR while changing your version of Vanilla to 4.16.0? It should update your static list trees to the new visuals without altering their behavior.

@ndv99
Copy link

ndv99 commented Oct 2, 2024

@jmuzina Is this still needed?

Yes, I haven't had the time yet to make this fix on maas-ui side and they are still on a rather old version of Vanilla, so it hasn't been a priority yet. Also, I've been having issues running maas-ui locally that I haven't resolved. But, it may be easier to get the ball rolling this way:

@ndv99 can you let me know what the priority is on updating maas UI to Vanilla 4.16.0, and try out the updates to .p-list-tree--static shown in this PR while changing your version of Vanilla to 4.16.0? It should update your static list trees to the new visuals without altering their behavior.

I've just put up a draft PR to upgrade Vanilla on maas-ui from 4.13 to 4.16: https://github.com/canonical/maas-ui/pull/5541Just waiting to get some CI results back and then I can give you a more accurate idea of how long it'll be. Happy to test this change in the mean time though!

And if you're struggling to get maas-ui running locally, we've recently updated our docs for how to get it running!

@ndv99
Copy link

ndv99 commented Oct 2, 2024

Also, for reference - we have one list tree in MAAS UI (to my knowledge), this is what it looks like before the fix is added (and vanilla upgraded):

And this is what it looks like after:

And if I comment out this line:

  .p-list-tree--static::before {
    @extend %list-tree-collapse-icon;
  }

I'll probably leave this line out in MAAS UI since we don't need to collapse this list tree, it's purely for controller status information. CI is looking good on the Vanilla upgrade PR, so we should be able to get that merged soon!

@jmuzina
Copy link
Member Author

jmuzina commented Oct 3, 2024

Closing as not needed since the changes here aren't really needed to get the MAAS UI list tree to work.

@jmuzina jmuzina closed this Oct 3, 2024
ndv99 added a commit to canonical/maas-ui that referenced this pull request Oct 15, 2024
- Upgraded vanilla to v4.16.0
- Added list tree fix from jmuzina (canonical/vanilla-framework#5336)
maas-lander pushed a commit to canonical/maas that referenced this pull request Oct 16, 2024
              chore(deps): Upgrade vanilla-framework to 4.16.0 (#5541)

- Upgraded vanilla to v4.16.0
- Added list tree fix from jmuzina (canonical/vanilla-framework#5336)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants