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

Reorganise props of multiline and localized inputs and fields #389

Merged
merged 1 commit into from
Jan 7, 2019

Conversation

dferber90
Copy link
Contributor

Unifies the APIs of multiline and localized inputs and fields.

Before

This is a list of prop-names each input is using. Notice that the respective field components use the same prop-names, and they have changed as well!

Concern MultilineTextInput LocalizedTextInput LocalizedMultilineTextInput
Picture image image image
Multiline Expansion isDefaultClosed isMultilineDefaultExpanded
Language Expansion isDefaultExpanded areLanguagesDefaultOpened
Language Controls hideExpansionControls hideLanguageControls

Now

Concern MultilineTextInput LocalizedTextInput LocalizedMultilineTextInput
Picture image image image
Multiline Expansion defaultExpandMultilineText defaultExpandMultilineText
Language Expansion defaultExpandLanguages defaultExpandLanguages
Language Controls hideLanguageExpansionControls hideLanguageExpansionControls

The same property renamings happened for LocalizedMultilineTextField, LocalizedTextField and MultilineTextField.

Notice that isDefaultClosed was renamed to defaultExpandMultilineText for MultilineTextInput, MultilineTextField, LocalizedMultilineTextInput and LocalizedMultilineTextField. This means the default has been flipped! You now need to provide defaultExpandMultilineText in cases where you were not passing isDefaultClosed to keep the same behaviour! It also means you can remove any places where you were passing isDefaultClosed={true} as that is the default now.

BREAKING

Changed APIs of LocalizedMultilineTextField, LocalizedTextField, MultilineTextField, LocalizedMultilineTextInput, LocalizedTextInput and MultilineTextInput.

Closes #313

Migration guide:

Flipped defaults

  • MultilineTextInput and MultilineTextField
    • isDefaultClosed was dropped. The multiline-text is now closed by default. Pass defaultExpandMultilineText to keep the original behaviour.

Simple renamings

  • LocalizedTextInput and LocalizedTextField
    • Rename isDefaultExpanded to defaultExpandLanguages
    • Rename hideExpansionControls to hideLanguageExpansionControls
  • LocalizedMultilineTextInput and LocalizedMultilineTextField
    • Rename isDefaultExpanded to defaultExpandLanguages
    • Rename hideExpansionControls to hideLanguageExpansionControls
    • Rename isMultilineDefaultExpanded to defaultExpandMultilineText

@dferber90 dferber90 changed the title feat: reorganize props of multiline and localized inputs and fields Reorganise props of multiline and localized inputs and fields Jan 4, 2019
Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

Good idea, thanks for making the APIs consistent 👌

@montezume
Copy link
Contributor

Something is weird with the visual diff in the multiline component

Copy link
Contributor

@montezume montezume left a comment

Choose a reason for hiding this comment

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

Looks like the MultilineTextField has different default behaviour, as shown here

@dferber90
Copy link
Contributor Author

dferber90 commented Jan 6, 2019

Something is weird with the visual diff in the multiline component

Yeah, the input/field is now closed by default instead of open by default. This was done to be able to have consistent prop names, as MultilineTextField was had multilines open by default while LocalizedMultilineTextField had multilines closed by default. Now they're all closed by default.

That's why they are all closed in VRT now. I found it slightly better to have the visual diff then to pass additional props. Now the VRTs show the minimal props again, otherwise they'd have to pass the optional prop on every test now..

TLDR: Let's accept the changes in Percy as they were made on purpose.

Copy link
Contributor

@montezume montezume left a comment

Choose a reason for hiding this comment

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

works for me

BREAKING: Changed APIs of LocalizedMultilineTextField, LocalizedTextField, MultilineTextField, LocalizedMultilineTextInput, LocalizedTextInput and MultilineTextInput
@dferber90 dferber90 merged commit 2d5a944 into master Jan 7, 2019
@dferber90 dferber90 deleted the df-unify-localized-apis branch January 7, 2019 08:02
dferber90 added a commit that referenced this pull request Jan 8, 2019
dferber90 added a commit that referenced this pull request Jan 8, 2019
montezume pushed a commit that referenced this pull request Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💅 Type: Enhancement Improves existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unify props of multiline / localized inputs
3 participants