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.

Sign up for GitHub

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

Jump to bottom

less brittle element type checks #2971

Merged
merged 6 commits into from
Oct 4, 2018
Merged

less brittle element type checks #2971

merged 6 commits into from
Oct 4, 2018

Conversation

giladgray
Copy link
Contributor

@giladgray giladgray commented Sep 25, 2018 •
edited
Loading

Fixes #2958

Changes proposed in this pull request:

  • ⚠️ Utils.isElementOfType now checks displayName equality to support multiple minor versions.
    • Utils are not part of the public API (though they are exported) so this will not be considered an API break
  • previously it created an element which is potentially expensive (for react-hot-loader support)
  • refactor Tabs and MultiSlider a little bit to support this new usage.

Reviewers should focus on:

  • does this still support react-hot-loader?

@blueprint-bot
Copy link

refactor Tabs to use type guard for children

Previews: documentation | landing | table

@giladgray
Copy link
Contributor Author

TODO: table components need displayNames

@blueprint-bot
Copy link

fix Tabs type guard

Previews: documentation | landing | table

@blueprint-bot
Copy link

add displayName fields to some Table components

Previews: documentation | landing | table

@blueprint-bot
Copy link

oops lint

Previews: documentation | landing | table

NeilRickards
NeilRickards approved these changes Sep 26, 2018
View reviewed changes
Copy link

@NeilRickards NeilRickards left a comment

Choose a reason for hiding this comment

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

I still find it a little odd that the Tab element isn't responsible for its own rendering, but this would appear to fix the issue I was seeing.

Worth having a review from someone more expert.

packages/core/src/common/utils.ts
element.type != null &&
element.type.displayName != null &&
element.type.displayName === ComponentType.displayName
);
Copy link

Choose a reason for hiding this comment

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

I'm looking forward to optional-chaining

@giladgray giladgray merged commit 839c14b into develop Oct 4, 2018
@giladgray giladgray deleted the gg/element-type-checks branch October 4, 2018 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@invliD invliD invliD approved these changes

@NeilRickards NeilRickards NeilRickards approved these changes

Assignees
No one assigned
Labels
Type: refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@giladgray @blueprint-bot @invliD @NeilRickards