-
Notifications
You must be signed in to change notification settings - Fork 172
ROX-31771: Fix user profile integration tests #18007
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
Conversation
|
Skipping CI for Draft Pull Request. |
|
@zhenpesky @mansursyed We seem to be using Even if I make the background gray, like in our PatternFly v5, it still doesn't look nice.
I'm wondering if using Tabs would be better here (see link). There seem to be vertical tabs and even an expandable tab with more visible active states. cc: @dvail @pedrottimark |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dv/ROX-28622-pf-6 #18007 +/- ##
=====================================================
- Coverage 49.35% 49.35% -0.01%
=====================================================
Files 2699 2699
Lines 198960 198960
=====================================================
- Hits 98199 98198 -1
Misses 93144 93144
- Partials 7617 7618 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@dvail never used git-spice before. Do I need to use it for the work we're doing? |
5a592fe to
8ce63f4
Compare
@mansursyed @zhenpesky Of note, I've already found a couple of other places where this is going to be a problem in the app, global search being another one. I really don't know why the PF team decided to make the active state nearly indistinguishable on these types of navigation items but it does reduce clarity in our UI. Depending on your guidance here, we might want to think on if there is an alternate pattern we can use and replace all instances where this is an issue. Possibly with Tabs like @sachaudh mentioned above. This might be a good topic for the UI/UX sync on Wed too. |
I don't think so, and honestly I'm not sure how this would work with both of us working on the same stack as git-spice keeps metadata locally on our development machines. I use it primarily because it makes stacking branches/PRs, changing between them, and rebasing the entire stack very easy. Now that we are working on individual page issues though, I think the stacks will be much shorter, and mostly targeting the PF6 feature branch directly (soon...), so stack management will be less of a concern. |
8ce63f4 to
cc491d0
Compare
65303ba to
69d9c49
Compare
403d48d to
69d9c49
Compare
|
@dvail, it looks like an earlier commit might have had the side effect of fixing this issue. It had auto-closed because I resolved the merge conflict by choosing the changes from the branch's head. I'll reopen for the visual changes, though. |
|
@mansursyed I do want to push this along, so I'll use the Vertical Tab and Expandable Tab approach for now. We can sync on further changes separately. |
|
@mansursyed @dvail, I wanted to brainstorm another option for that page. I initially tried using Tabs, but it felt odd to have a Vertical Tab and a Vertical Expandable Tab. It would have looked nicer with just one or the other, but together it still looked weird. Another option could be a Selector. Since the user roles list is dynamic, it should work well as a select option.
@mansursyed I know you wanted some time to think about this one. Let me know if any of the previous suggestions work for you, or if you have any others. I'm ok with whatever is most consistent and simplest 👍🏼 |
Signed-off-by: Saif Chaudhry <schaudhr@redhat.com>
Signed-off-by: Saif Chaudhry <schaudhr@redhat.com>
Signed-off-by: Saif Chaudhry <schaudhr@redhat.com>
Signed-off-by: Saif Chaudhry <schaudhr@redhat.com>
Signed-off-by: Saif Chaudhry <schaudhr@redhat.com>
dvail
left a comment
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.
We should consider whether or not the loss of routing, and potential saved bookmarks, is a worthwhile tradeoff too.
Usually I would try to avoid losing it, but do we anticipate many saved bookmarks to specific roles in your profile? Is there much of a use case for doing so?
- Rename label to 'User permissions for all roles' for clarity - Use '##ALL_ROLES##' constant to prevent collision with API role names Signed-off-by: Saif Chaudhry <schaudhr@redhat.com>
Signed-off-by: Saif Chaudhry <schaudhr@redhat.com>
Signed-off-by: Saif Chaudhry <schaudhr@redhat.com>
Signed-off-by: Saif Chaudhry <schaudhr@redhat.com>


Description
This PR fixes the user profile page integration tests.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
Before:
After: