-
Notifications
You must be signed in to change notification settings - Fork 178
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
[RHOAI-14745]: Apply Final Microcopy Changes #3403
[RHOAI-14745]: Apply Final Microcopy Changes #3403
Conversation
0a7864c
to
85f2193
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3403 +/- ##
==========================================
+ Coverage 85.94% 85.96% +0.01%
==========================================
Files 1339 1339
Lines 30310 30323 +13
Branches 8369 8378 +9
==========================================
+ Hits 26051 26068 +17
+ Misses 4259 4255 -4
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
id="no-connections-tooltip" | ||
content="No existing connections available" | ||
id="no-connection-types-tooltip" | ||
content="No connection types available" |
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 are attaching connections and not connection types
.
content="No connection types available" | |
content="No connections available" |
Furthermore, the create button may be disabled if there are no connection types.
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 @christianvogt , nice catch!
Wondering if we should then add another tooltip to the disabled create button for that case when there are no connection types, i.e.:
Currently if a user clicks the create button and toggles the modal, both the dropdown and the create button in the modal are disabled when there are no connection types:
When you have a moment would be great to get your thoughts on the best approach here @simrandhaliw
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.
@jenny-s51, we should disable the button and display the tooltip upon hover. The tooltip text above also looks good to me.
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 @simrandhaliw , updated
Create connection | ||
</Button> | ||
{enabledConnectionTypes.length === 0 && ( | ||
<Tooltip | ||
id="no-connection-types-tooltip" | ||
content="No connection types available" | ||
triggerRef={tooltipRef} | ||
/> | ||
)} |
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 also have an Add connection
that gets disabled just the same if there's no connection types. But right now there's no tooltip.
@simrandhaliw i think this button should also be named Add connection
, no? Other pages don't mix create
and add
actions labels.
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.
All instances of connection creation should use "Create" instead of "Add," since the opposite action is "Delete". If the opposite action were "Remove," then "Add" would be appropriate.
Also, we should update the preview drawer's modal heading from "Add connection" to "Create connection," as this is consistent with the terminology used for connection creation modals.
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 also have an Add connection that gets disabled just the same if there's no connection types. But right now there's no tooltip.
@christianvogt Do you mean the "Create connection" action in the Connections tab of the Project details view? Yes, that is another instance where the same tooltip would be displayed for the disabled button.
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.
There's several inconsistencies with other tabs. Perhaps it's best to create an issue to align them all instead of part of this PR.
frontend/src/pages/projects/screens/detail/connections/ConnectionsList.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/projects/screens/spawner/connections/ConnectionsFormSection.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/projects/screens/spawner/connections/ConnectionsFormSection.tsx
Show resolved
Hide resolved
c525c96
to
b43187b
Compare
Create connection | ||
</Button> | ||
{enabledConnectionTypes.length === 0 && ( | ||
<Tooltip content="No connection types available" triggerRef={tooltipRef} /> |
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.
Missing ID to go with the aria-describedby
attribute.
<Tooltip content="No connection types available" triggerRef={tooltipRef} /> | |
<Tooltip id="no-connection-types-tooltip" content="No connection types available" triggerRef={tooltipRef} /> |
Also noticed that the Add connection
button doesn't have a tooltip when there is an existing connection but no connection types.
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 @christianvogt - applied these fixes
fa4dfc6
to
4b414fd
Compare
remove unused import disable create connection button and show tooltip on hover remove key, add aria-describedBy,format remove duplicate ID on button fix aria-describedBy check to match disabled check add ID, apply tooltip to disabled Add Connection button
4b414fd
to
cbd718e
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: christianvogt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
https://issues.redhat.com/browse/RHOAIENG-14745
Description
Before
Environment Variable: Default Help Text
Environment Variable: Error Text
Environment Variable: Warning Text
"Attach existing connection" button tooltip in workbench creation form:
"Create connection" button in Connections tab of project view (tooltip does not exist here):
After
Environment Variable: Default Help Text
Environment Variable: Error Text
Environment Variable: Warning Text
'
"Attach existing connections" button tooltip in "Create workbench" form:
"Create connection" button tooltip in Connections tab of project view:
How Has This Been Tested?
Add field modal:
Connection Types -> "Edit" in kebab dropdown for connection type -> "Add field"
In workbench creation form:
Disable all connection types -> Navigate into a DSP -> Create workbench -> Verify "Attach existing connections" button is disabled and has correct microcopy on tooltip when hovered: "No connection types available".
In "Connections" tab of a DSP:
Disable all connection types -> Navigate into a DSP -> Go to "Connections" tab -> Verify "Create connection" button is disabled and has correct microcopy on tooltip when hovered: "No connection types available".
Test Impact
N/A - updates to microcopy
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main