-
Notifications
You must be signed in to change notification settings - Fork 2
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
Refactor Select documentation to no longer mention SelectNext #1620
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1620 +/- ##
===========================================
- Coverage 100.00% 99.90% -0.10%
===========================================
Files 62 62
Lines 1071 1071
Branches 410 410
===========================================
- Hits 1071 1070 -1
- Misses 0 1 +1 ☔ View full report in Codecov by Sentry. |
@@ -489,7 +497,7 @@ export const Select = Object.assign( | |||
// Calling the function directly instead of returning a JSX element, to | |||
// avoid an unnecessary extra layer in the component tree | |||
// eslint-disable-next-line new-cap | |||
return SelectNext({ ...props, multiple: false }); | |||
return SelectMain({ ...props, multiple: false }); |
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 can save one step here by directly calling SelectMain
, which is going to be kept as the internal component long-term.
2807c2a
to
e03abdc
Compare
Codecov is reporting a drop in code coverage, but I think it's a false positive. Locally, it reports a coverage of 100%, and if you go to codecov's report, there's no line highlighted as uncovered. |
e03abdc
to
acf7bea
Compare
src/pattern-library/components/patterns/prototype/SelectPage.tsx
Outdated
Show resolved
Hide resolved
@@ -196,10 +196,10 @@ const routes: PlaygroundRoute[] = [ | |||
route: '/input-option-button', | |||
}, | |||
{ | |||
title: 'SelectNext', | |||
title: 'Selects', |
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.
title: 'Selects', | |
title: 'Select', |
All the other menu items are named after the main component on that page, so I think we should do the same here.
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.
acf7bea
to
8b45429
Compare
This PR replaces the
/input-select-next
documentation page with/input-select
, which has very similar content and examples, but always usingSelect
andMultiSelect
and no longer mentioningSelectNext
.Additionally, it deprecates
SelectNext
, and recommends using one of the other two components as drop-in replacements.Technically speaking, two breaking changes are introduced here:
/input-select-next
URL will no longer work. I guess we could potentially make it redirect to/input-select
if we think this is a big problem.Option
component'sdisplayName
has changed from'SelectNext.Option'
to'Select.Option'
, which could make some test break, but I already have branches almost ready to transition away from SelectNext anyway, so it shouldn't be a big problem.Select.Option
function reference, rather than via its display name.