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

Mrc 4808 Add Base Select #4

Merged
merged 10 commits into from
Feb 22, 2024
Merged

Mrc 4808 Add Base Select #4

merged 10 commits into from
Feb 22, 2024

Conversation

M-Kusumgar
Copy link
Contributor

@M-Kusumgar M-Kusumgar commented Dec 14, 2023

Still won't be able to see anything in the demo yet, but we are very close. This is the base for both the single select and the multi select.

Thanks @EmmaLRussell for the suggestion to use mixins in #2. It uses a simple mixin to extract out the expand, collapse and flattenOptions logic. The structure of it is very similar to the single select component you guys reviews in #1 however note the slot in the c-dropdown-toggle, that will come in handy for when we have to display the tags for the multiselect! The other difference is that the dropdown-item component is now also given this conditional checked prop. Otherwise I believe everything in the template is the same as the single select so hopefully shouldnt be too bad to review.

@M-Kusumgar M-Kusumgar changed the base branch from main to setup December 14, 2023 15:23
Copy link

codecov bot commented Dec 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (33ead02) 100.00% compared to head (2dd7c08) 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           mrc-4811        #4   +/-   ##
==========================================
  Coverage    100.00%   100.00%           
==========================================
  Files             2         6    +4     
  Lines            25        88   +63     
  Branches          7        25   +18     
==========================================
+ Hits             25        88   +63     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@M-Kusumgar M-Kusumgar changed the base branch from setup to main December 14, 2023 15:52
@M-Kusumgar M-Kusumgar changed the base branch from main to setup December 14, 2023 15:54
@M-Kusumgar M-Kusumgar changed the base branch from setup to mrc-4811 December 14, 2023 16:19
@M-Kusumgar M-Kusumgar marked this pull request as ready for review December 14, 2023 16:42
Copy link
Member

@r-ash r-ash left a comment

Choose a reason for hiding this comment

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

This looks really great, and really tidy.

Couple of comments/questions.

Are you going to add a test harness here or something that we can run up and look at the different selects?

expandOptions(flatOptions.value, optionPath);
};

const collapse = (optionPath: string[]) => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const collapse = (optionPath: string[]) => {
const collapse = (optionPath: string[]) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

vite.config.ts Outdated
@@ -10,7 +10,7 @@ export default defineConfig({
css: true,
coverage: {
provider: "istanbul",
include: ["src/components"],
include: ["src/components", "src/utils.ts"],
Copy link
Member

Choose a reason for hiding this comment

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

mixins dir too? Is there an easier way to do to this, maybe include all src but exclude the files we don't want to test?

Choose a reason for hiding this comment

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

Agreed, feels safer to include everything be default, then exclude selectively, otherwise to easy to forget to include new code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, am just including src now

@@ -0,0 +1,123 @@
<template>
<div>
<c-dropdown auto-close="outside"
Copy link
Member

Choose a reason for hiding this comment

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

Do you want this for the base select? I thought clicking inside for a single select should close the dropdown?

With this set what is the behaviour when you select an item from the drop down?

Copy link
Member

Choose a reason for hiding this comment

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

Looking at this in the single select PR looks like this isn't an issue. When we click within the single select it is closing nicely, where that closing is handled by SingleSelect. Is this set to outside here to that when you click an arrow in the hierarchy it does not close the dropdown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exactly, this is there so the dropdown doesnt automatically close when we click the chevrons!

@r-ash
Copy link
Member

r-ash commented Jan 11, 2024

Are you going to add a test harness here or something that we can run up and look at the different selects?

Just seen this in the next PR!

@M-Kusumgar M-Kusumgar changed the base branch from mrc-4811 to main February 22, 2024 11:59
@M-Kusumgar M-Kusumgar merged commit 4c12880 into main Feb 22, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants