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: Group logically related props together (#242) ⚡️ #242

Merged
merged 15 commits into from
May 9, 2019

Conversation

ellinge
Copy link
Collaborator

@ellinge ellinge commented Apr 18, 2019

What does it do?

Bundles text props and "mode" (radio/simple) into a single object/prop

  • Breaking change
  • This change requires a documentation update

@coveralls
Copy link

coveralls commented Apr 18, 2019

Pull Request Test Coverage Report for Build 1150

  • 22 of 22 (100.0%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.09%) to 94.315%

Totals Coverage Status
Change from base Build 1147: -0.09%
Covered Lines: 566
Relevant Lines: 581

💛 - Coveralls

@mrchief mrchief changed the title fix: Bundle of props BREAKING CHANGE: Group logically related props together Apr 22, 2019
@mrchief
Copy link
Collaborator

mrchief commented Apr 22, 2019

Can you base it off a branch in the repo itself? Or else update your fork for the latest prlint.json (the former is preferable though)

@mrchief
Copy link
Collaborator

mrchief commented Apr 22, 2019

Ok, seems like it's not needed. Updating the branch did the trick. That makes me wonder if we should change that check (to be based off develop always)... Food for thought.

@mrchief mrchief changed the title BREAKING CHANGE: Group logically related props together feat: Group logically related props together May 9, 2019
# Conflicts:
#	__snapshots__/src/tree-node/index.test.js.snap
BREAKING: Property changes

| Description                             | Usage before                                                | Usage after                                                   |
| --------------------------------------- | ----------------------------------------------------------- | ------------------------------------------------------------- |
| Added a new `mode` prop                 | `simpleSelect={true}` / `simpleSelect`                      | `mode='simpleSelect'`                                         |
| Bundled text props into a single object | `placeholderText='My text'`<br>`noMatchesText='No matches'` | `texts={{ placeholder: 'My text', noMatches: 'No matches' }}` |
@codeclimate
Copy link

codeclimate bot commented May 9, 2019

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

View more on Code Climate.

@mrchief mrchief changed the title feat: Group logically related props together feat: Group logically related props together (#242) ⚡️ May 9, 2019
@mrchief mrchief merged commit 6a632cc into dowjones:develop May 9, 2019
@ellinge ellinge deleted the fix/BundleOfProps branch May 9, 2019 20:35
@ellinge
Copy link
Collaborator Author

ellinge commented May 9, 2019

@mrchief nothing else we should bundle? showDropdown?

@ellinge
Copy link
Collaborator Author

ellinge commented May 9, 2019

Also the doc changes is not reflected in the d.ts/typings

@mrchief
Copy link
Collaborator

mrchief commented May 9, 2019

@mrchief nothing else we should bundle? showDropdown?

I haven't considered any other props yet but let me check and get back to you on that. I'll hold off on releasing a new version until then.

Also the doc changes is not reflected in the d.ts/typings

The doc in there looked Ok to me. Do you mean they are not the same verbatim?

@ellinge
Copy link
Collaborator Author

ellinge commented May 9, 2019

Everything else is based up on the doc’s formulation so just a check so that you simply just did not forget.

@mrchief
Copy link
Collaborator

mrchief commented May 9, 2019

I got distracted by the fact that the readme is over 10K char limit now. The all-contributors table is probably the biggest culprit. Need to rethink it now.

@mrchief
Copy link
Collaborator

mrchief commented May 10, 2019

@ellinge Looking at the types, I was wondering why we are documenting props again there. Is it standard practice in the typescript community?

@ellinge
Copy link
Collaborator Author

ellinge commented May 10, 2019

They are used by the IDE for intellisense (in case of vs code/visual studio) just as documented props in C#.

@ellinge
Copy link
Collaborator Author

ellinge commented May 10, 2019

bild
@mrchief vs code

@mrchief
Copy link
Collaborator

mrchief commented May 10, 2019

Hmmm, I suspected as much. I wonder if there's a way to DRY up this stuff without going over the top. Sometimes duplication is simpler.

mrchief pushed a commit that referenced this pull request May 21, 2019
BREAKING CHANGE: Property changes

| Description                             | Usage before                                                | Usage after                                                   |
| --------------------------------------- | ----------------------------------------------------------- | ------------------------------------------------------------- |
| Added a new `mode` prop                 | `simpleSelect={true}` / `simpleSelect`                      | `mode='simpleSelect'`                                         |
| Bundled text props into a single object | `placeholderText='My text'`<br>`noMatchesText='No matches'` | `texts={{ placeholder: 'My text', noMatches: 'No matches' }}` |
mrchief added a commit that referenced this pull request Jun 10, 2019
* feat: Group logically related props together (#242) ⚡️

BREAKING CHANGE: Property changes

| Description                             | Usage before                                                | Usage after                                                   |
| --------------------------------------- | ----------------------------------------------------------- | ------------------------------------------------------------- |
| Added a new `mode` prop                 | `simpleSelect={true}` / `simpleSelect`                      | `mode='simpleSelect'`                                         |
| Bundled text props into a single object | `placeholderText='My text'`<br>`noMatchesText='No matches'` | `texts={{ placeholder: 'My text', noMatches: 'No matches' }}` |
mrchief pushed a commit that referenced this pull request Jun 10, 2019
BREAKING CHANGE: Property changes

| Description                             | Usage before                                                | Usage after                                                   |
| --------------------------------------- | ----------------------------------------------------------- | ------------------------------------------------------------- |
| Added a new `mode` prop                 | `simpleSelect={true}` / `simpleSelect`                      | `mode='simpleSelect'`                                         |
| Bundled text props into a single object | `placeholderText='My text'`<br>`noMatchesText='No matches'` | `texts={{ placeholder: 'My text', noMatches: 'No matches' }}` |
mrchief added a commit that referenced this pull request Jun 10, 2019
* feat: Group logically related props together (#242) ⚡️

BREAKING CHANGE: Property changes

| Description                             | Usage before                                                | Usage after                                                   |
| --------------------------------------- | ----------------------------------------------------------- | ------------------------------------------------------------- |
| Added a new `mode` prop                 | `simpleSelect={true}` / `simpleSelect`                      | `mode='simpleSelect'`                                         |
| Bundled text props into a single object | `placeholderText='My text'`<br>`noMatchesText='No matches'` | `texts={{ placeholder: 'My text', noMatches: 'No matches' }}` |
mrchief pushed a commit that referenced this pull request Jun 10, 2019
BREAKING CHANGE: Property changes

| Description                             | Usage before                                                | Usage after                                                   |
| --------------------------------------- | ----------------------------------------------------------- | ------------------------------------------------------------- |
| Added a new `mode` prop                 | `simpleSelect={true}` / `simpleSelect`                      | `mode='simpleSelect'`                                         |
| Bundled text props into a single object | `placeholderText='My text'`<br>`noMatchesText='No matches'` | `texts={{ placeholder: 'My text', noMatches: 'No matches' }}` |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants