Skip to content
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

feat: [UX-332] Add multiselect support #448

Merged
merged 7 commits into from
Oct 17, 2023
Merged

feat: [UX-332] Add multiselect support #448

merged 7 commits into from
Oct 17, 2023

Conversation

witwash
Copy link
Contributor

@witwash witwash commented Oct 3, 2023

Summary

  • Added multiselect support for the OptimusSelectInput
  • Refactored OptimusSelectInput
  • Updated story
Video
CleanShot.2023-10-09.at.22.49.57.mp4

Testing steps

  • Open Select Input story
  • Enable multiselect
  • Test the component using different variants of exposed options.

Follow-up issues

Refactoring 🔧

Check during review

  • Verify against Jira issue.
  • Is the PR over 300 additions? Consider rejecting it with advice to split it. Is it over 500 additions? It should definitely be rejected.
  • Unused code removed.
  • Build passing.
  • Is it a bug fix? Check that it is covered by a proper test (unit or integration).

@witwash witwash added the ignore-size Ignore the size of this PR label Oct 3, 2023
@witwash witwash marked this pull request as ready for review October 10, 2023 09:10
@witwash witwash requested a review from a team October 10, 2023 09:10
@ookami-kb
Copy link
Contributor

@witwash can refactoring be done as a separate PR to make this PR smaller?

@witwash
Copy link
Contributor Author

witwash commented Oct 11, 2023

@ookami-kb, here is just the multiselect part. The refactoring will come in the follow-up. This is kinda the best I can do. New multiselect is using the same parts as the regular input, but is not input per se, so I had to take out some common parts.

@ookami-kb
Copy link
Contributor

@witwash ah, I thought you could do refactoring PR with extracting some "future"-common part as a pre-step.

@witwash
Copy link
Contributor Author

witwash commented Oct 17, 2023

@ookami-kb, can you look at this one please? 🙏

Copy link
Contributor

@ookami-kb ookami-kb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments, otherwise, LGTM.

@@ -90,6 +92,14 @@ class OptimusSelectInput<T> extends StatefulWidget {
/// {@endtemplate}
final GroupBuilder? groupBuilder;

/// Whether to enable having several items selected at the same time.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rephrase it to something like

If true, you can select multiple items at the same time.

@@ -90,6 +92,14 @@ class OptimusSelectInput<T> extends StatefulWidget {
/// {@endtemplate}
final GroupBuilder? groupBuilder;

/// Whether to enable having several items selected at the same time.
/// State of the selected items is manages outside this widget and has to be
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// State of the selected items is manages outside this widget and has to be
/// State of the selected items is managed outside this widget and has to be

bool get _isUsingInlineError =>
widget.errorVariant == OptimusInputErrorVariant.inlineTooltip;

void _onStateUpdate() => setState(() {});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_handleStateUpdate

@witwash witwash enabled auto-merge (squash) October 17, 2023 13:13
@witwash witwash merged commit 48fa669 into master Oct 17, 2023
@witwash witwash deleted the multiselect branch October 17, 2023 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ignore-size Ignore the size of this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants