-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[24.2] Fix inconsistent tab handling in invocation view #19298
[24.2] Fix inconsistent tab handling in invocation view #19298
Conversation
client/src/components/WorkflowInvocationState/WorkflowInvocationMetrics.vue
Outdated
Show resolved
Hide resolved
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.
nice!
looks like a round of prettier is needed |
…onMetrics.vue Co-authored-by: Marius van den Beek <m.vandenbeek@gmail.com>
74100e5
to
4532464
Compare
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 for the unification!
I personally think that the exclamation badge and disabling the tab is a bit confusing.
For these reasons:
- It draws a lot of attention "for a normal situation" and makes me think there is a problem in those tabs until I read the tooltip.
- The visual state for a disabled tab is not very noticeable (besides for the extra exclamation badge) and requires extra code and style.
Those are not meant to be a -1, just my subjective opinion on UX 😅
client/src/components/WorkflowInvocationState/WorkflowInvocationState.vue
Outdated
Show resolved
Hide resolved
Co-authored-by: David López <46503462+davelopez@users.noreply.github.com>
@davelopez @mvdbeek and folks in general, is this better? There's even the primary variant: |
Still looks good to me, I think the primary variant is more appopriate ? |
Moved the badge out to the tabs-end slot! ✅ @davelopez (updated screencast in description) |
Yes, this looks way better to me! Thank you! The export invocation selenium test might need an update to wait for the correct event now that the tab is disabled 😅 |
This is a very beautiful enhancement. So much polish! |
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.
Thank you! 🙏
Fixes #19286
consistent_tabs_invocation_view_2.mp4
Initial (outdated) change, with the added badge to the export tab
consistent_tabs_invocation_view.mp4
How to test the changes?
(Select all options that apply)
License