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: Add more explanatory texts for empty states #12354

Merged
merged 6 commits into from
Apr 11, 2022

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Feb 15, 2022

Related: #10426 (comment)

This PR does two things:

  1. From a human perspective it updates all our 'empty state' texts to include information on the ACL policy dispositions required to view data on that specific view.
  2. Moves all of this related text to our translation files.

Small note here:

I noticed whilst doing this that we seem to be very close to full i18n'ed text in all our Routes now. In fact we are probably at a stage where we should make a move to specifically go ahead and make sure all our Routes are 100% i18n'ed seeing as we must be about 95% there, we may aswell go ahead and get the last 5% done soon-ish.

@github-actions github-actions bot added the theme/ui Anything related to the UI label Feb 15, 2022
@vercel vercel bot temporarily deployed to Preview – consul February 16, 2022 10:58 Inactive
@johncowen johncowen marked this pull request as ready for review February 16, 2022 11:00
@johncowen johncowen requested review from natmegs, jgwhite, a user and amyrlam February 16, 2022 11:07
{{t "routes.dc.services.instance.upstreams.tproxy-mode.footer"}}
</Action>
</p>
{{t "routes.dc.services.instance.upstreams.tproxy-mode.footer"}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these two might need htmlSafe=true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh great catch! Lemme fix that up now 👍

@johncowen
Copy link
Contributor Author

This has a little conflict now, likely to be done after the weekend will ping properly once its ready again

@johncowen
Copy link
Contributor Author

johncowen commented Feb 25, 2022

😬 YAML conflicts are far more hairy than they should be. I have a few improvements in the works that I've not been able to PR as yet for various reasons that would help here. Pretty sure this one is correct now, looking at the diff here there was a bit of moving around of yaml-ness.

@natmegs I guess it's ready for review now , but I'll be holding off on the merge for a little while whichever.

@johncowen
Copy link
Contributor Author

🎉 Thanks for the approval!

Just a quick note here, there are a couple of questions outstanding on the textual content here so just gonna leave this hanging a little more until we get clarity on that. We may just merge and follow up with an additional PR or just pop more on that end here. Following up offline.

@johncowen
Copy link
Contributor Author

Just to follow up here, I noticed the test we have added here might not be correct, if I've understood correctly the text we are using here only specifies that you need service:read for the service listing page, whereas according to the user below you need a few more policy dispositions in order to see that view.

#10426 (comment)

It turns out that the anonymous token needs to also have read access to "node" and/or "agent". Should have been obvious that it needs it since it displays info about the nodes where the service is registered 🙄

So I've checked in with the team to see whether to merge this or wait until we get corrected text (if it does need correcting)

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

@vercel vercel bot temporarily deployed to Preview – consul-ui-staging April 4, 2022 09:53 Inactive
@vercel vercel bot temporarily deployed to Preview – consul April 4, 2022 09:53 Inactive
@johncowen johncowen force-pushed the ui/feature/empty-state-texts branch from 7a4d770 to b7d701d Compare April 4, 2022 09:53
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging April 4, 2022 09:53 Inactive
@vercel vercel bot temporarily deployed to Preview – consul April 4, 2022 09:53 Inactive
@johncowen
Copy link
Contributor Author

I tried this out and node:read is required for viewing the service listing page, so I added that piece of text. Another rebase was required which needed a little bit of manual yaml'ing also, so that all went in the same commit.

Let me know if anything else needs to happen here, otherwise I'll leave it hanging for a little while longer then merge.

@johncowen johncowen merged commit e622756 into main Apr 11, 2022
@johncowen johncowen deleted the ui/feature/empty-state-texts branch April 11, 2022 11:50
@hc-github-team-consul-core
Copy link
Collaborator

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/632517.

@hc-github-team-consul-core
Copy link
Collaborator

🍒✅ Cherry pick of commit e622756 onto release/1.12.x succeeded!

hc-github-team-consul-core pushed a commit that referenced this pull request Apr 11, 2022
* ui: Add more explanatory texts for empty states

* Change all template "Read the guide"s

* Add missing htmlSafe

* Remove the stuff I commented out to try and grok the hairy rebase

* Changelog

* More rebased yaml weirdness plus added node:read
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/no-metrics-test theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants