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

New layout for mobile menu is ready. #2665

Merged
merged 28 commits into from
Sep 10, 2019
Merged

Conversation

YegorSan
Copy link
Contributor

@YegorSan YegorSan commented Sep 4, 2019

This PR is related to #2653 issue.

Motivation

New layout for mobile menu was requested.

Changelog

Created a new menu layout for mobile view.
*Not active state:

*Screenshot (1)

Active state:

Screenshot (12)

Checklist for your PR

Copy link
Member

@vbaranov vbaranov left a comment

Choose a reason for hiding this comment

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

@YegorSan as with previous PR too many unnecessary changes. Please check https://github.com/poanetwork/blockscout/pull/2665/files. Also, you can check which changes do you make before committing them.

@YegorSan
Copy link
Contributor Author

YegorSan commented Sep 4, 2019

@vbaranov I have re-checked the files, should be better now. I am sorry, again VSCode turned on formatting for css files automatically.

@YegorSan
Copy link
Contributor Author

YegorSan commented Sep 5, 2019

@vbaranov Sorry for such a huge list of commits. Lot of tests didn't pass yesterday (ESLINT, test_geth_http_websocket, etc). I've fixed all the issues.

@vbaranov vbaranov self-requested a review September 5, 2019 10:36
Copy link
Member

@vbaranov vbaranov left a comment

Choose a reason for hiding this comment

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

@YegorSan @pashagonchar

  1. I think elements in the header shouldn't jump on open menu https://www.loom.com/share/86f4fe08df8343b494054f478a390e83

  2. Menu items are not centred as in @pashagonchar screenshot in the issue Menu mobile view #2653. I don't know should they be centred finally or not. @pashagonchar please check
    Screenshot 2019-09-05 at 13 53 52

  3. The green dot is not vertically centred
    Screenshot 2019-09-05 at 13 57 08

Screenshot 2019-09-05 at 14 01 26

@@ -30,7 +44,7 @@
</span>
<%= gettext("Blocks") %>
</a>
<div class="dropdown-menu" aria-labelledby="navbarBlocksDropdown">
<div class="dropdown-menu" id="checkIfSmall" aria-labelledby="navbarBlocksDropdown">
Copy link
Member

Choose a reason for hiding this comment

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

@YegorSan unnecessary trailing whitespace

@pashagonchar
Copy link

@vbaranov
Menu items are not centred as in @pashagonchar screenshot in the issue #2653. I don't know should they be centred finally or not. @pashagonchar please check

Yes, I updated mockup in zeplin

image

@YegorSan
Copy link
Contributor Author

YegorSan commented Sep 5, 2019

@vbaranov Hello. I am working on performing mentioned above changes right now. Will try to resolve in the shortest terms.

@YegorSan
Copy link
Contributor Author

YegorSan commented Sep 5, 2019

@vbaranov Hi Victor. Just finished with the requested updates. All changes were performed. Please review. Thank you in advance.

@vbaranov vbaranov self-requested a review September 6, 2019 11:50
margin-right: -7px !important;
padding-right: 0px;
height: 20px!important;
border-left: 1px solid #828ba0a1!important;
Copy link
Member

Choose a reason for hiding this comment

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

@YegorSan is the correct color hex here? It looks like it is invalid colour hex. It should be 3-byte hexadecimal code

@vbaranov
Copy link
Member

vbaranov commented Sep 6, 2019

@YegorSan in addition, elements are still jumping https://www.loom.com/share/71119ba076624298aeace00fc02d2a5e

@vbaranov
Copy link
Member

vbaranov commented Sep 6, 2019

@YegorSan in addition, I don't see dark mode switcher in the desktop mode anymore

Screenshot 2019-09-06 at 15 37 22

@YegorSan
Copy link
Contributor Author

YegorSan commented Sep 7, 2019

@vbaranov Hello. I've added fixed height for logo (was max-height: 28px now height: 28px). Jumping issue is resolved now.
https://www.loom.com/share/3b9d72fdf09f4954a2333d838c40d101
I was not able to reproduce this issue locally as Blockscout main logo appearing by default, when I switched to Ethereum logo - issue was reproduced. Should be better now. Also changed color to 3-byte hexadecimal code, and dark mode switcher is appearing now in desktop mode. Please review. Thank's in advance.

@vbaranov
Copy link
Member

vbaranov commented Sep 9, 2019

@YegorSan please resolve merging conflicts

@YegorSan
Copy link
Contributor Author

YegorSan commented Sep 9, 2019

@vbaranov Merging conflicts seems to be solved. Could you please review? Thank's in advance.

@vbaranov
Copy link
Member

@YegorSan changelog entry...

@YegorSan
Copy link
Contributor Author

@vbaranov I am sorry. Changelog is updated.

@vbaranov vbaranov self-requested a review September 10, 2019 10:11
Copy link
Member

@vbaranov vbaranov left a comment

Choose a reason for hiding this comment

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

@YegorSan

Dark mode switcher is not clickable in a mobile view

@YegorSan
Copy link
Contributor Author

@vbaranov Hello Victor. I am sorry again, forgot to add separate JS onClick function for new button created. Now button is clickable and working as it expected.

@vbaranov vbaranov self-requested a review September 10, 2019 12:06
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

3 participants