Add helper text to ReleaseSelect#4232
Conversation
|
This is a follow up to #4226 |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The helper text element uses a hardcoded
id='release-select-helper'which could lead to duplicate IDs ifReleaseSelectis rendered more than once on a page; consider generating a unique ID per instance (e.g., via a hook or prop) and wiringaria-describedbyto that.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The helper text element uses a hardcoded `id='release-select-helper'` which could lead to duplicate IDs if `ReleaseSelect` is rendered more than once on a page; consider generating a unique ID per instance (e.g., via a hook or prop) and wiring `aria-describedby` to that.
## Individual Comments
### Comment 1
<location path="src/Components/CreateImageWizard/steps/ImageOutput/tests/ReleaseSelect.test.tsx" line_range="47-53" />
<code_context>
expect(screen.getByText('*')).toBeInTheDocument();
});
+
+ test('shows helper text', () => {
+ renderReleaseSelect();
+
+ expect(
+ screen.getByText(/latest version is selected by default/i),
+ ).toBeInTheDocument();
</code_context>
<issue_to_address>
**suggestion (testing):** Add an assertion that the helper text is correctly wired via `aria-describedby` for accessibility
Since the helper text is explicitly linked via `aria-describedby="release-select-helper"`, please add an assertion that the select element exposes this relationship (e.g., checking the `aria-describedby` value or using an a11y matcher like `toHaveAccessibleDescription`).
```suggestion
test('shows helper text', () => {
renderReleaseSelect();
const helperText = screen.getByText(/latest version is selected by default/i);
expect(helperText).toBeInTheDocument();
const releaseSelect = screen.getByRole('combobox', { name: /release/i });
expect(releaseSelect).toHaveAccessibleDescription(
/latest version is selected by default/i,
);
});
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/Components/CreateImageWizard/steps/ImageOutput/tests/ReleaseSelect.test.tsx
Show resolved
Hide resolved
The mocks have some helper text below the release select to say that the latest version of a release is always the default.
This is a great time to be updating the aliases since we're touching this file for the revamp.
e1724e3 to
c0ada85
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #4232 +/- ##
=======================================
Coverage 73.68% 73.68%
=======================================
Files 198 198
Lines 7261 7263 +2
Branches 2688 2690 +2
=======================================
+ Hits 5350 5352 +2
Misses 1672 1672
Partials 239 239
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
mgold1234
left a comment
There was a problem hiding this comment.
/lgtm
just a small nit pick
not sure if the dot at the end of the helper text is needed,
I am working on Timeszone revamp step and there is no dot at the end.
but there are other step that include the dot at the end.
maybe we should check with Shayna?
Fair point. I guess we can ask on the next UI/UX call. I just copied the mocks to be honest and the full stop was there already 🤷♂️ |
Summary
Adds helper text below the release select dropdown to inform users that the latest version is selected by default, matching the design mocks. Also converts deep relative imports to
@/aliases.Changes
FormHelperTextcomponentaria-describedbyfor screen reader accessibility../../../../../) to@/alias imports