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

Sticky position the tabs at the top #8609

Merged
merged 1 commit into from
May 18, 2020
Merged

Conversation

whymarrh
Copy link
Contributor

This PR updates the home screen tabs to stick to the top of the screen when scrolling.

Gif

(It's slow to start, give it a second.)

Gif of the transaction list scrolling with the tabs at the top
Screenshots

@whymarrh whymarrh requested a review from a team as a code owner May 18, 2020 16:10
rekmarks
rekmarks previously approved these changes May 18, 2020
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

:guy-fieri-chef-kiss:

@Gudahtt
Copy link
Member

Gudahtt commented May 18, 2020

What does this look like on the fullscreen view?

Edit: Ah right - no tabs on the fullscreen view.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

This doesn't seem to work well with the token cells:

Screenshot:

overlapped-tokens

@Gudahtt
Copy link
Member

Gudahtt commented May 18, 2020

I'm not sure about this being set globally on the tab component 🤔

Could this have an effect the other places that component is used? I don't recall any of the other tab contents being long enough to scroll, so maybe not.

@whymarrh
Copy link
Contributor Author

This doesn't seem to work well with the token cells:

Fixed.

Could this have an effect the other places that component is used? I don't recall any of the other tab contents being long enough to scroll, so maybe not.

I've check most of the other usages and they're all fine, namely the add token page and the gas customization modal. (I don't know currently know the conditions under which the confirm page will have tabs.)

I'm not sure about this being set globally on the tab component 🤔

sticky will only affect usages where the tabs scrolled with the content, which shouldn't be the case anywhere else. This seems like a safe default. But I could also add it to just the home tabs.

@Gudahtt
Copy link
Member

Gudahtt commented May 18, 2020

I don't know currently know the conditions under which the confirm page will have tabs

It has tabs when confirming a contract interaction iirc. The second tab is "data", which could be pretty long. I can try to reproduce it.

@Gudahtt Gudahtt dismissed their stale review May 18, 2020 17:10

This has been addressed

@whymarrh
Copy link
Contributor Author

It has tabs when confirming a contract interaction

Ah, thanks. From the test dapp:

Screen Shot 2020-05-18 at 14 42 00

It doesn't scroll but if it did:

Screen Shot 2020-05-18 at 14 43 36

I think this is a lot better as well.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@whymarrh whymarrh merged commit 3485bfb into MetaMask:develop May 18, 2020
@whymarrh whymarrh deleted the sticky-tabs branch May 18, 2020 18:52
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.

4 participants