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
chore: add close button to troubleshooting and help page #4457
chore: add close button to troubleshooting and help page #4457
Conversation
d1e685c
to
628561d
Compare
@mairin The original design was no breadcrumbs on these two pages, but it sounds like it's throwing people off. Thoughts? @jannikbertram If we do go down this route we should remove the property entirely instead of setting it true everywhere, and likely rename Home to Dashboard since I assume that's where it goes 'back' to. |
Another option would be to only include the ❌ icon and remove the breadcrumbs. |
@deboer-tim @jannikbertram Help and Troubleshooting didn't have breadcrumbs because they sort of belong to the status bar... Neither has a presence on the dashboard (not that they shouldn't, they just don't) so semantically linking back to the Dashboard as a parent in a breadcrumb feels weird. I do think the X in the upper right is a good idea and may have even been in the original design. I really like how you have it going to the last previously-viewed screen when the user hits the X. I would say let's try a light touch first and just do the X, and try to side step the 🤯-ness of the breadcrumbs for now? |
Thanks for clarifying this @mairin. I added a new commit and updated the PR description. This is how it looks when you use the X button together with actions and breadcrumbs: I'm not sure if it should be possible like this. I feel like this really is about small details, so please let me know if you want this to behave differently. I can keep updating this until we find a consistent solution |
That looks odd to me. I don't think this PR should change the location of the close button on all the other pages (it should remain on the 'breadcrumb bar'), only add a close button to the ones that don't have breadcrumbs. |
26979c6
to
bc0c70f
Compare
I updated the PR again
now it simply does this ☝️ |
@jannikbertram this looks great to me, just tested it out :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix unit tests
bc0c70f
to
03129d7
Compare
Signed-off-by: Jannik Bertram <jb@allgravy.com>
Signed-off-by: Jannik Bertram <jb@allgravy.com>
Signed-off-by: Jannik Bertram <jb@allgravy.com>
Signed-off-by: Jannik Bertram <jb@allgravy.com>
ff7ead8
to
ce9a424
Compare
@jeffmaury can you trigger the unit tests again? They should be working now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @jannikbertram for your contribution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @jeffmaury, assuming you're 👍🏼 now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @jannikbertram for this
What does this PR do?
Screenshot/screencast of this PR
What issues does this PR fix or reference?
Closes #2878
How to test this PR?
Visit the "Troubleshooting" and "Help" page