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

ui: Token listing redesign #8117

Merged
merged 3 commits into from
Jun 17, 2020
Merged

ui: Token listing redesign #8117

merged 3 commits into from
Jun 17, 2020

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Jun 16, 2020

This PR is a redesign of the token listing page.

Screenshot 2020-06-16 at 13 01 15

This necessitated various things, and I'll be adding various self-review comments to try to aid review here:

  1. Slight reworking of %composite-list to add a CSS grid layout to allow us to have a right hand grid cell for the actions button.
  2. Mirrored the CSS grid layout with 3 new block slots for ListComponent
  3. Migrated our copy-button component to use StateChart and Tooltip components. This also meant we could delete the FeedbackDialog component whose functionality was pretty much a state machine anyway.
  4. Move all usages of ListComponent into higher order components named by their 'consul type' i.e. ConsulSomethingList and migrate those to use the new block slots.

Potential future work/improvements:

  1. Tooltips seem to be slightly problematic in hard to realize areas, such as control over accessibility features and testing.
  2. Add pageobjects and READMEs to all new presentational components.
  3. Move all references to %more-popover-menu to the new CSS files specifically for this component.

I've PRed this now as the changeset was growing more than I'd like, but I'll likely to be looking into ^ at some point in the future.

@johncowen johncowen added the theme/ui Anything related to the UI label Jun 16, 2020
@johncowen johncowen force-pushed the ui/feature/token-list-redesign branch from 4f1130e to d72382f Compare June 16, 2020 12:04
Copy link
Contributor Author

@johncowen johncowen left a comment

Choose a reason for hiding this comment

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

📓

content: ', ';
white-space: pre;
content: ',';
margin-right: 0.3em;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that the tag-listing wasn't wrapping properly when not in listings, this fixes that up.

%definition-table > dl > * {
min-height: 1em;
margin-bottom: 0.4em;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now we are using plain dls for other things. I moved what was our 'default layout' for dls to a 'definition table' component. This means we shouldn't have to keep overwriting dl margins/padding anymore.

%instances-row > * {
width: calc(100% / 4);
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Managed to delete quite a lot of the old table centric CSS for service, instance and token listings.

@@ -1,4 +1,5 @@
@setupApplicationTest
@ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to ignore this test in the end, not massively happy about that, but ember-tooltips causes the tests to hang here for a while.

@@ -77,7 +77,7 @@ Feature: dc / acls / tokens / index: ACL Token List
s: Si-Search
---
And I see 1 token model
And I see 1 token model with the serviceIdentity "Si-Search"
And I see 1 token model with the serviceIdentity "Service Identity: Si-Search"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, I'm not massively happy with this change as it depends on the design of the service identity display. I decided to PR this instead of getting deeper into this and adding more and more change here.

@johncowen johncowen requested a review from kaxcode June 16, 2020 12:12
@johncowen johncowen marked this pull request as ready for review June 16, 2020 12:12
Copy link
Contributor

@kaxcode kaxcode left a comment

Choose a reason for hiding this comment

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

🎨 🔥 ♻️ LGTM

@johncowen johncowen force-pushed the ui/feature/token-list-redesign branch from 44ccfb8 to 751f99e Compare June 17, 2020 08:50
@johncowen johncowen merged commit 7522bd5 into master Jun 17, 2020
@johncowen johncowen deleted the ui/feature/token-list-redesign branch June 17, 2020 09:25
@hashicorp-ci
Copy link
Contributor

🍒❌ Cherry pick of commit 7522bd5 onto release/1.8.x failed! Build Log

johncowen added a commit that referenced this pull request Jun 17, 2020
johncowen pushed a commit that referenced this pull request Jul 10, 2020
We changed our default definition list layout in
#8117.

We replaced the default with a definition-table class but missed one
place where the old default was previously used.

This adds the definition-table class in RTT where it used to use the
default.
johncowen added a commit that referenced this pull request Jul 10, 2020
We changed our default definition list layout in
#8117.

We replaced the default with a definition-table class but missed one
place where the old default was previously used.

This adds the definition-table class in RTT where it used to use the
default.
hashicorp-ci pushed a commit that referenced this pull request Jul 10, 2020
We changed our default definition list layout in
#8117.

We replaced the default with a definition-table class but missed one
place where the old default was previously used.

This adds the definition-table class in RTT where it used to use the
default.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants