Header/Menu improvement #607

fairlighteth · 2022-05-31T08:30:51Z

Summary

Final merge PR to merge in the waterfall PR series, the final one being Header/Menu improvement (waterfall PR FINAL) Header/Menu improvement (waterfall PR FINAL) #596

github-actions · 2022-05-31T08:31:06Z

CLA Assistant Lite:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

I have read the CLA Document and I hereby sign the CLA

_{You can retrigger the CLA Action by commenting recheckcla in this Pull Request}

github-actions · 2022-05-31T08:41:30Z

🔭 GP Swap: CoW Protocol v2 Swap UI

src/custom/hooks/useChangeNetworks.ts

nenadV91

Code wise LGTM. I just want to point out one UI issue I've noticed, if this is fixed in some of the following PR's please ignore this

* Move onclick for balance + wallet * Improve header 3 (#605) * Rename /profile to /account * Improve header 4 (#604) * Switch to custom MenuDropdown compontent and create a POC. * Improve header 5 (#603) * Improve FAQ text * Improve header 6 (#602) * connect button styling * Further flyout menu styling * Improve header 7 (#601) * Further flyout menu styling * Uniform menu items styling * Move styled components to styled * Add hamburger icon with animation component. * Improve header 8 (#600) * Add mediaQuery hook * Improve header 9 (#599) * Progress mobile responsive menu * Progress mobile responsive menu * Improve header 10 (#598) * Move to helper function * Optimise menu code * Improve header 11 (#597) * Move URLS to an ENUM * Fix enum issue. * Styling and route fixes. * Styling. * Styling. * Header/Menu improvement (waterfall PR FINAL) (#596) * Styling. * Styling footer and bridge banner. * Improve header 13 (#610) * Fix network selector position. * Fix exports/imports for network selector. * Only open ordersPanel if account is true.

W3stside · 2022-06-03T09:47:23Z

src/custom/components/Header/MobileMenuIcon/index.tsx

+import styled, { css } from 'styled-components/macro'
+
+const Wrapper = styled.div<{ isMobileMenuOpen: boolean; height?: number; width?: number; lineSize?: number }>`
+  z-index: 102;


102 😬😬😬

Yeah I know...I'd want to tackle the z-index thing in a separate PR. I think moving it to a const file, and then defining some general levels would be better.

W3stside · 2022-06-03T09:52:57Z

src/custom/components/Header/MobileMenuIcon/index.tsx

+  margin: 0 6px 0 16px;
+  position: relative;
+  width: ${({ width }) => (width ? width + 'px' : '34px')};
+  height: ${({ height }) => (height ? height + 'px' : '18px')};


In these kinds of props i prefer setting the default in the function param destructuring like:

width: ${({ width = 18 }) => `${width}px`};

It just looks cleaner and is less code

Ah nice, of course this is way cleaner setting the default like that!

W3stside · 2022-06-03T09:54:05Z

src/custom/components/Header/MobileMenuIcon/index.tsx

+  span {
+    background-color: ${({ theme }) => theme.text1};
+    border-radius: 3px;
+    height: ${({ lineSize }) => (lineSize ? lineSize + 'px' : '2px')};


I won't mention all of these instances but could also be replaced here

W3stside · 2022-06-03T09:56:39Z

src/custom/components/Header/MobileMenuIcon/index.tsx

+
+export default function MobileMenuIcon({ isMobileMenuOpen, width, height, lineSize, onClick }: IconProps) {
+  return (
+    <Wrapper isMobileMenuOpen={isMobileMenuOpen} width={width} height={height} lineSize={lineSize} onClick={onClick}>


Since all the prop names are the same you can just not destructure the props in the function signature and pass them to the component like <Component {...params} />

W3stside · 2022-06-03T10:00:13Z

src/custom/components/MenuDropdown/index.tsx

+export function Menu({ title, children }: MenuProps) {
+  const isUpToLarge = useMediaQuery(upToLarge)
+  const [showMenu, setShowMenu] = useState(false)
+  const handleOnClick = () => isUpToLarge && setShowMenu(!showMenu)


SetStates allow passing a callback as a param so it's safer to do:

setShowMenu(menuState => !menuState)

Like this ?
const handleOnClick = () => isUpToLarge && setShowMenu((showMenu) => !showMenu)

Why is it safer?

W3stside · 2022-06-03T10:02:07Z

src/custom/constants/routes.ts

@@ -0,0 +1,24 @@
+// ENUM with routes


Woo nice good call!

W3stside · 2022-06-03T10:11:50Z

src/custom/utils/toggleBodyClass.ts

@@ -0,0 +1,7 @@
+export const toggleBodyClass = (className: string, isAdd: boolean) => {


toggle in the name implies inverting the current state so the isAdd param is redundant

this function can just be written as

export const toggleBodyClass = (className: string) => { if (!document.body.classList.contains(className)) { document.body.classList.add(className) } else { document.body.classList.remove(className) } }

Makes sense yes. Actually I created also specific addBodyClass and removeBodyClass util functions, as that seems sometimes more convenient.

W3stside

approving with minor comments

* Updating util function * change onClick onTouchStart

elena-zh · 2022-06-07T11:33:18Z

Hey @fairlighteth , great job!

Some issues I faced during testing:

I noticed that some links remain to be underlined when navigate to another FAQ section:
Seems like 'Protocol' section is missing in the FAQ now: 'Token' section is opened when press on the 'Protocol'

3 FAQ overview link is always highlighted, even if another section is selected

WDYT about auto-closing dropdowns after selecting this or that option? Currently, dropdown remains opened unless I remove a mouse from it
I can't open a menu in a tablet/mobile view (Win10+Chrome, FF, Brave)
Maybe we should display feedback window above the feedback icon, not from the right side? WDYT?

At least, it will not overlap Connect wallet button
Footer in a tablet view hides contracts/block/gas price info
Connect to wallet button has a light boarder in the light mode
The issue with FAQ mobile sections' alignment 'Protocol' FAQ section is expanded more than the rest sections in a mobile view #486 is also reproducible in this PR
It is almost impossible to scroll down the menu in iPhone with iOS 15.3

menu.mp4

Account Affiliate section elements exceed the section's height in iPhone 12 mini (when a user is not connected)

in Android 'Connect wallet' button is shifted to the right
Maybe just like a reminder: the issue with the feedback window for iOS wallets integration browsers is still broken ([iOS] - enable Feedback modal in imToken integrated browser #165)
And as an enhancement I'd like to propose to show COW and ETH balances at the top of the menu (however, do not have a strong opinion on this)

Thanks!

fairlighteth · 2022-06-07T13:58:01Z

Yes, this is to indicate the current active page in the sub-menu. In this case I will change it to a different color instead

2 and 3: Addressed this and point 1 now to better handle this.

Yes makes sense. I now made sure that on click of any menu element is closes the dropdown menu. On click anywhere outside the menu > it closes as well. Also instead of mouse-over, I made it to open the dropdown on click of the parent element (with the arrow).

Addressed these in #650

fairlighteth · 2022-06-07T13:59:17Z

I think what happens is that the onClick events aren't refreshed when you change viewport. Can you try again by refreshing the page? If it does work then we need to address that issue.

elena-zh · 2022-06-07T14:06:10Z

I think what happens is that the onClick events aren't refreshed when you change viewport. Can you try again by refreshing the page? If it does work then we need to address that issue.

@fairlighteth , refreshing does not help :(
https://watch.screencastify.com/v/uuW5RvCrudYQR9kWMA81

* Fix iOS scrolling attempt and improve events * Fix iOS scrolling attempt and improve events

* Resolve FAQ exact links and styling submenu. * Unify FAQ menu links by importing them. * close dropdowns on click and on click outside.

* Re-position Appzi modal on mobile. * FAQ adjust modal * FAQ adjust modal * try catching Appzi root ID. * try catching Appzi root ID. * try catching Appzi root ID. * try catching Appzi root ID. * try catching Appzi root ID. * fix account margin * uncomment * simple overrides * restore original code

elena-zh · 2022-06-09T11:05:36Z

Hey @fairlighteth , all issues are mostly fixed!
As I promised, I will create new tasks for unfixed issues, but I'd like to draw your attention to this one (I think, it is an important one): swap form boarders are missing in a mobile view:

there are no margins on small screen resolutions:

Could you please take a look at it?

Thanks!

fairlighteth · 2022-06-09T11:17:35Z

@elena-zh Thanks for checking again. The borders were intentionally removed to allow for more horizontal space. By doing that, the container/content area has more available width to display content/numbers.

elena-zh · 2022-06-09T11:29:30Z

@fairlighteth , got it, thanks!
But maybe then we should remove the bottom boarder, WDYT?

fairlighteth · 2022-06-09T11:35:29Z

@elena-zh Actually I would leave that one in place.

elena-zh · 2022-06-09T13:09:26Z

@fairlighteth , I have created:
#668 : new issue
#669 : issue related to Account page Connect button

Btw, I'm still not sure if it is OK that we have a white boarder for the Connect button in the light mode?

Btw, where is Terms and Conditions page now?

Thanks

fairlighteth · 2022-06-09T14:50:30Z

@elena-zh

Indeed forgot to add the terms and conditions -> add terms conditions #670
When merging the Accounts page PR at some point, I will also check the 'view orders' menu item.
In regard to the white border on the button, personally I think this is fine (intentional that is).

elena-zh · 2022-06-10T10:18:39Z

Hey @fairlighteth ,
after the final round of testing I have reported some new/missing issues:

Tablet: appzi modal is closing effect is shifted to the right #672 (new)
Add COW and ETH/XDAI balances to the menu in a mobile view #673 (I have missed it)
'ETH/XDAI' in the balance is cropped in a tablet view #276 (comment) - added a note to the existing issue.

Besides, I still vote for fixing #669 in the near future.

But for now, the current PR is approved.

Thanks!

Update right header elements styling.

afa768c

fairlighteth requested review from a team May 31, 2022 08:31

W3stside reviewed May 31, 2022

View reviewed changes

src/custom/hooks/useChangeNetworks.ts Outdated Show resolved Hide resolved

W3stside approved these changes May 31, 2022

View reviewed changes

nenadV91 approved these changes May 31, 2022

View reviewed changes

fairlighteth added 2 commits June 2, 2022 16:08

Merge conflict

6a57a12

W3stside reviewed Jun 3, 2022

View reviewed changes

W3stside approved these changes Jun 3, 2022

View reviewed changes

Improve header 1 updates (#628)

52395d3

* Updating util function * change onClick onTouchStart

conflict resolve

7110cc3

fairlighteth mentioned this pull request Jun 7, 2022

Improve header 1 elena feedback #650

Merged

fairlighteth and others added 5 commits June 7, 2022 16:23

Add footer margin on small and medium resolutions. (#652)

294a355

FAQ adjust margin (#653)

d8b0ece

Fix iOS scrolling mobile menu overlay attempt and improve events (#656)

83d1a00

* Fix iOS scrolling attempt and improve events * Fix iOS scrolling attempt and improve events

Improve header 1 elena feedback (#650)

5b1bb94

* Resolve FAQ exact links and styling submenu. * Unify FAQ menu links by importing them. * close dropdowns on click and on click outside.

Fix code style issues with Prettier

c28ec83

fairlighteth added 2 commits June 8, 2022 17:24

fix touch event issue.

ec8f96c

This was referenced Jun 9, 2022

[Firefox] App footer is transparent on reduced screens #668

Closed

Account: 'Connect wallet' button is too big, right-aligned and overlaps contracts in a mobile view #669

Closed

fairlighteth added 2 commits June 9, 2022 16:43

add terms conditions (#670)

32f9b75

resolve merge conflict

1540f70

This was referenced Jun 10, 2022

Tablet: appzi modal is closing effect is shifted to the right #672

Closed

Add COW and ETH/XDAI balances to the menu in a mobile view #673

Closed

'ETH/XDAI' in the balance is cropped in a tablet view #276

Closed

elena-zh approved these changes Jun 10, 2022

View reviewed changes

fairlighteth merged commit ffe64f9 into develop Jun 14, 2022

github-actions bot locked and limited conversation to collaborators Jun 14, 2022

alfetopito deleted the improve-header-1 branch June 17, 2022 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Header/Menu improvement #607

Header/Menu improvement #607

fairlighteth commented May 31, 2022

github-actions bot commented May 31, 2022 •

edited

Loading

github-actions bot commented May 31, 2022

nenadV91 left a comment

W3stside Jun 3, 2022

fairlighteth Jun 3, 2022

W3stside Jun 3, 2022

fairlighteth Jun 3, 2022

W3stside Jun 3, 2022

W3stside Jun 3, 2022

W3stside Jun 3, 2022

fairlighteth Jun 3, 2022

W3stside Jun 3, 2022

W3stside Jun 3, 2022

fairlighteth Jun 3, 2022

W3stside left a comment

elena-zh commented Jun 7, 2022

fairlighteth commented Jun 7, 2022

fairlighteth commented Jun 7, 2022

elena-zh commented Jun 7, 2022

elena-zh commented Jun 9, 2022

fairlighteth commented Jun 9, 2022

elena-zh commented Jun 9, 2022

fairlighteth commented Jun 9, 2022

elena-zh commented Jun 9, 2022

fairlighteth commented Jun 9, 2022

elena-zh commented Jun 10, 2022

		@@ -0,0 +1,7 @@
		export const toggleBodyClass = (className: string, isAdd: boolean) => {

Header/Menu improvement #607

Header/Menu improvement #607

Conversation

fairlighteth commented May 31, 2022

Summary

github-actions bot commented May 31, 2022 • edited Loading

github-actions bot commented May 31, 2022

nenadV91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

W3stside left a comment

Choose a reason for hiding this comment

elena-zh commented Jun 7, 2022

fairlighteth commented Jun 7, 2022

fairlighteth commented Jun 7, 2022

elena-zh commented Jun 7, 2022

elena-zh commented Jun 9, 2022

fairlighteth commented Jun 9, 2022

elena-zh commented Jun 9, 2022

fairlighteth commented Jun 9, 2022

elena-zh commented Jun 9, 2022

fairlighteth commented Jun 9, 2022

elena-zh commented Jun 10, 2022

github-actions bot commented May 31, 2022 •

edited

Loading