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

refactor: Update mode to include hierarchical #251

Merged
merged 8 commits into from
May 12, 2019
Merged

Conversation

mrchief
Copy link
Collaborator

@mrchief mrchief commented May 10, 2019

BREAKING: hierarchical prop

hierarchical prop is now moved under mode prop.

// before
<DropdownTreeSelect data={data} hierarchical={true} />

// after
<DropdownTreeSelect data={data} mode="hierarchical" />

@coveralls
Copy link

coveralls commented May 10, 2019

Pull Request Test Coverage Report for Build 1159

  • 4 of 4 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.315%

Totals Coverage Status
Change from base Build 1151: 0.0%
Covered Lines: 566
Relevant Lines: 581

💛 - Coveralls

@mrchief mrchief requested a review from ellinge May 10, 2019 02:40
@ellinge
Copy link
Collaborator

ellinge commented May 10, 2019

Just checked on my mobile. Will check further later. Looks good 👍 You need to update the story for options.

Should we do something about showShopdown, shopDropdownAlways.
What about oneOf(true, false, 'always') or something breaking for true/false as well.

The options tree/search props (keep*) could be bundled also? Perhaps one should not go over the top though with the bundling. Thoughts?

ellinge
ellinge previously approved these changes May 10, 2019
Copy link
Collaborator

@ellinge ellinge left a comment

Choose a reason for hiding this comment

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

Should be alright I we don't see anything else to bundle, but we could do that in a separate PR instead.

@mrchief
Copy link
Collaborator Author

mrchief commented May 10, 2019

The options tree/search props (keep*) could be bundled also? Perhaps one should not go over the top though with the bundling. Thoughts?

I concur. I don't see anything else as a natural candidate for bundling.

Should be alright I we don't see anything else to bundle, but we could do that in a separate PR instead.

Agreed. The showDropdown bundling will be a separate PR. Thinking about showDropdown: 'initial' | 'always'

Good catch about updating the Options story. Will do that.

@mrchief
Copy link
Collaborator Author

mrchief commented May 10, 2019

@ellinge Do you know if we absolutely need the semis in the .ts file? Prettier keeps removing them and I'm not sure if that's an error.

@mrchief
Copy link
Collaborator Author

mrchief commented May 10, 2019

This uses a mix of semis, commas, and no semis and doesn't seem to cause any syntax errors. According to this old issue, semis are optional too. If so, we can let prettier loose and strip the semis. Thoughts?

@ellinge
Copy link
Collaborator

ellinge commented May 10, 2019

Go ahead and strip them. I think we’ll be just fine and concistent.

@codeclimate
Copy link

codeclimate bot commented May 10, 2019

Code Climate has analyzed commit 8f84986 and detected 0 issues on this pull request.

View more on Code Climate.

@ellinge
Copy link
Collaborator

ellinge commented May 12, 2019

Still works great within vs code if that was what you intended me to verify after dismissing my review.

@mrchief mrchief merged commit 8d064fb into develop May 12, 2019
@mrchief
Copy link
Collaborator Author

mrchief commented May 12, 2019

Still works great within vs code if that was what you intended me to verify after dismissing my review.

There's a GitHub setting that auto dismisses previous reviews if you push new commits. :)

@mrchief mrchief deleted the doc/hierarchical branch May 12, 2019 15:35
mrchief added a commit that referenced this pull request May 21, 2019
BREAKING CHANGE: `hierarchical` prop

`hierarchical` prop is now moved under `mode` prop.

```
// before
<DropdownTreeSelect data={data} hierarchical={true} />

// after
<DropdownTreeSelect data={data} mode="hierarchical" />
```
mrchief added a commit that referenced this pull request Jun 10, 2019
BREAKING CHANGE: `hierarchical` prop

`hierarchical` prop is now moved under `mode` prop.

```
// before
<DropdownTreeSelect data={data} hierarchical={true} />

// after
<DropdownTreeSelect data={data} mode="hierarchical" />
```
mrchief added a commit that referenced this pull request Jun 10, 2019
BREAKING CHANGE: `hierarchical` prop

`hierarchical` prop is now moved under `mode` prop.

```
// before
<DropdownTreeSelect data={data} hierarchical={true} />

// after
<DropdownTreeSelect data={data} mode="hierarchical" />
```
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.

None yet

3 participants