Skip to content

This should fix bug in Tabs (react-toolbox@1.3.4 ) with incorrect pointer update #1567

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

Merged
merged 1 commit into from
Aug 2, 2017

Conversation

timurgarif
Copy link

Hi!
I use master branch (react-toolbox@1.3.4). The bug affects that version.

Bug description.

When the Tabs component contains some Tab components that vary depending on an app state, pointer element (under active tab) could be rendered with incorrect width.
This doesn't affect initial render, only render after state changes that affect Tab components.
E.g. if an active tab receives a new label prop that has longer or shorter text than current label then Tabs.updatePointer in Tabs.componentWillReceiveProps will calculate width at nextProps.index (which is correct), but based on old tab element (which is incorrect).
the first state example: first-state
the second state example: second-state
Notice how the blue pointer on the second example repeats the width of the pointer from the first example (previous tabs state).

Suggested solution.

  1. In Tabs component remove componentWillReceiveProps method.
  2. In Tabs component add componentDidUpdate method as shown in the pull request.

dev branch (2.x)

As I noticed, componentDidUpdate has been added like in my suggestion. That's nice but I think componentWillReceiveProps should be removed there. Though I cannot now test and prove for this version, it most probably triggers unnecessary render for the old tabs and pointer that will be rerendered by state change in componentDidUpdate anyway.

@javivelasco javivelasco merged commit 50accd4 into react-toolbox:master Aug 2, 2017
@javivelasco
Copy link
Member

Thanks!

david-slayte pushed a commit to david-slayte/react-toolbox that referenced this pull request Jul 6, 2018
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.

2 participants