DACCS-Climate / marble-website

https://marbleclimate.com
0 stars 1 forks source link

Marble mobile menu #122

Closed alexandercyu closed 6 months ago

alexandercyu commented 6 months ago
mishaschwartz commented 6 months ago

@alexandercyu please resolve the merge conflicts and then I'll have another look

alexandercyu commented 6 months ago

@alexandercyu please resolve the merge conflicts and then I'll have another look

The conflicts have been resolved.

alexandercyu commented 6 months ago

Looks good, when close button is implemented move the close button down on Y axis

Do you mean the 'X' button to close the mobile menu? That's already been implemented. What is the distance you want it moved down on the Y axis?

alexandercyu commented 6 months ago

I'm still waiting for a response to several comments from my previous review.

Either make changes, or leave a comment: explaining why you don't think the requested change is necessary, asking for a clarification, suggesting an alternative ...

Don't just ignore the review comments please.

Sorry, didn't mean to ignore those. It's just my habit of looking at the ones I get emails for. I should be looking at the entire list.

Anyway, those are done now.

ShrutiK100 commented 6 months ago

Looks good, when close button is implemented move the close button down on Y axis

Do you mean the 'X' button to close the mobile menu? That's already been implemented. What is the distance you want it moved down on the Y axis?

Looks good, when close button is implemented move the close button down on Y axis

Do you mean the 'X' button to close the mobile menu? That's already been implemented. What is the distance you want it moved down on the Y axis?

Have created a new issue about this. Please check.

alexandercyu commented 6 months ago

Aligned hamburger and close buttons in mobile menu at their top left corners. Fixes #123

Changed opacity for close button in mobile menu.

alexandercyu commented 6 months ago
  • On sm and md screen sizes the quick start menu is now all the way to the left (no padding or margins at all) which I'm pretty sure we don't want.

    • In Figma, the mobile menu items are horizontally aligned in the middle of the screen. Here, they are aligned to the left

    • In Figma, the close button is above the "MARBLE" title, not beside it

mishaschwartz commented 6 months ago

@alexandercyu I'll review after conflicts have been resolved please

alexandercyu commented 6 months ago

@alexandercyu I'll review after conflicts have been resolved please

The Resolve Conflicts button is greyed out for me. Does that mean I have to do it locally instead of online?

mishaschwartz commented 6 months ago

Yes:

This branch has conflicts that must be resolved Use the command line to resolve conflicts before continuing.

alexandercyu commented 6 months ago

Yes:

This branch has conflicts that must be resolved Use the command line to resolve conflicts before continuing.

Ok, I've resolved the conflicts.

mishaschwartz commented 6 months ago

The close button has changed since initial design. It is now a bit left and a bit above the Marble title.

Yes, but the close button is still not fully above the title.

image

in Figma:

image

alexandercyu commented 6 months ago

The close button has changed since initial design. It is now a bit left and a bit above the Marble title.

Yes, but the close button is still not fully above the title.

image

in Figma:

image

I've added more space between the close button and title.