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 and convert Tabs component from CoffeeScript to JS #2549

Merged
merged 5 commits into from
Feb 5, 2018

Conversation

jhawthorn
Copy link
Contributor

Previously we had logic to see whether the container had grown or shrunk, and responded by either taking elements out of the dropdown or moving them in.

Instead, this commit combines the separate code paths and every time the width is changed removes all elements from the DOM and one-by-one re-inserts them into the correct parent.

This shouldn't be significantly slower then the previous behaviour because we are looking at very few DOM nodes and are careful to not force layout at any point in the loop.

We also convert Tabs from CoffeeScript to plain JS, as well as use the DOM directly instead instead of jQuery.

@jhawthorn jhawthorn requested a review from graygilmore February 1, 2018 23:22
Previously we were using display: none to hide this element, which made
it impossible to discover its outerWidth when it was hidden.

This allows us to remove a hack which assumed it was 50px wide (which is
never actually true) when we weren't able to discover its width.
Previously we had logic to see whether the container had grown or
shrunk, and responded by either taking elements out of the dropdown or
moving them in.

Instead, this commit combines the separate code paths. Every time the
width is changed all elements are removed from the DOM and one-by-one
re-inserted into the correct parent.

This shouldn't be significantly slower then the previous behaviour.
We are looking at very few DOM nodes and are careful to not force layout
at any point in the loop.
A minor nitpick, but we don't need jQuery here. In a number of places we
were using the underlying objects (like $el[0]) so we might as well
stick with those throughout.
@jhawthorn jhawthorn merged commit f619342 into solidusio:master Feb 5, 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.

None yet

4 participants