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

feat: navbar redesign #4063

Merged
merged 25 commits into from
Oct 18, 2022
Merged

feat: navbar redesign #4063

merged 25 commits into from
Oct 18, 2022

Conversation

Jarsen136
Copy link
Contributor

@Jarsen136 Jarsen136 commented Oct 3, 2022

Thank you for your contribution to the KodaDot NFT gallery.
👇 _ Let's make a quick check before the contribution.

PR type

  • Bugfix
  • Feature
  • Refactoring

What's new?

  • PR closes Redesign: Navbar #3946

  • Redesign: Navbar - UI

  • Redesign: Navbar - Dark Mode

  • Redesign: Navbar - Responsive

  • Redesign: Navbar - Add new searchbar component

Before submitting Pull Request, please make sure:

  • My contribution builds clean without any errors or warnings
  • I've merged recent default branch -- main and I've no conflicts
  • I've tried to respect high code quality standards
  • I've didn't break any original functionality
  • I've posted a screenshot of demonstrated change in this PR

Optional

  • I've tested it at </rmrk/collection/26902bc2f7c20c546a-1FVG7>
  • I've tested PR on mobile and everything seems works
  • I found edge cases
  • I've written some unit tests 🧪

Had issue bounty label?

  • Fill up your KSM address: Payout

Community participation

Screenshot

  • My fix has changed something on UI; a screenshot is best to understand changes for others.

image

image

@Jarsen136 Jarsen136 requested review from a team as code owners October 3, 2022 04:41
@Jarsen136 Jarsen136 requested review from prachi00 and petersopko and removed request for a team October 3, 2022 04:41
@kodabot
Copy link
Collaborator

kodabot commented Oct 3, 2022

WARNING @Jarsen136 PR for issue #3946 which isn't assigned to you. Please be warned that this PR may get rejected if there's another assignee for issue #3946

@netlify
Copy link

netlify bot commented Oct 3, 2022

Deploy Preview for koda-nuxt ready!

Name Link
🔨 Latest commit cfcad32
🔍 Latest deploy log https://app.netlify.com/sites/koda-nuxt/deploys/634e33a252e74400098789ab
😎 Deploy Preview https://deploy-preview-4063--koda-nuxt.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@prachi00
Copy link
Member

prachi00 commented Oct 3, 2022

Screen Shot 2022-10-02 at 10 58 41 PM

btn text should be black in light mode, reference to design:

Screen Shot 2022-10-02 at 10 59 05 PM

@Jarsen136
Copy link
Contributor Author

btn text should be black in light mode, reference to design:

✅ Fixed
image

image

@petersopko
Copy link
Contributor

image

I remember there already was an issue regarding the searchbar in navbar, referencing the text (it's too long and doesn't get shortened on smaller screen sizes). Can we change it here and change the Search by Collection or NFT to just Search so the text stays readable and isn't cut? 🙏🏻

@petersopko
Copy link
Contributor

@Jarsen136 I saw the change you made, I apologize for not phrasing it correctly, I'd like to see the placeholder shortened on smaller screen sizes, not everywhere

@Jarsen136
Copy link
Contributor Author

image

I remember there already was an issue regarding the searchbar in navbar, referencing the text (it's too long and doesn't get shortened on smaller screen sizes). Can we change it here and change the Search by Collection or NFT to just Search so the text stays readable and isn't cut? 🙏🏻

Sure.

✅ Fixed
image

@Jarsen136
Copy link
Contributor Author

@Jarsen136 I saw the change you made, I apologize for not phrasing it correctly, I'd like to see the placeholder shortened on smaller screen sizes, not everywhere

OK, I would take a look

@Jarsen136
Copy link
Contributor Author

Jarsen136 commented Oct 3, 2022

✅ Fixed

small screen:
image

normal:
image

@petersopko
Copy link
Contributor

@Jarsen136 I've noticed just now that there are two more buttons compared to the design in figma, am I missing something ? (sorry if this was mentioned somewhere, couldn't find it).
image

I assume the chain selection is moved to the new landing under search bar and I can't seem to find the language dropdown new position 🤔 @exezbcz

Copy link
Contributor

@petersopko petersopko left a comment

Choose a reason for hiding this comment

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

some questions

@exezbcz
Copy link
Member

exezbcz commented Oct 3, 2022

hey @petersopko, yup, that's right, network selection is removed because landing and the whole app will show nfts from all networks. Language isn't there because we are thinking about putting it into profile settings once you connect your wallet. The thing I will add is the dark mode switch button. I will add it in one day. Thanks!

@petersopko
Copy link
Contributor

hey @petersopko, yup, that's right, network selection is removed because landing and the whole app will show nfts from all networks. Language isn't there because we are thinking about putting it into profile settings once you connect your wallet. The thing I will add is the dark mode switch button. I will add it in one day. Thanks!

so I guess we're keeping all of those controls in navbar for now until further redesign happens in the future?

if yes, @Jarsen136 could we change the design of the buttons to have sharper edges and not be rounded-like (in the style of redesign):
image

@exezbcz
Copy link
Member

exezbcz commented Oct 4, 2022

We can keep the language and dark mode in. With the language selection redesigned, I would remove the chevron.

@Jarsen136
Copy link
Contributor Author

If I understand you correctly, this is what we expected.

image

@Jarsen136 Jarsen136 marked this pull request as draft October 15, 2022 13:53
@Jarsen136
Copy link
Contributor Author

Jarsen136 commented Oct 15, 2022

TODO:

  • language select
  • dark mode
  • mobile version of navbar(functionality)
  • UI of mobile version

@Jarsen136
Copy link
Contributor Author

The functionality of navbarV2 on mobile is already as same as that on pc. However, there still more need to be done on mobile according to the redesign Figma.
Because UI structure and interaction are quite different from pc, we should implement a new component MobileNavbarDropdown according to Navbar component.

now:
image

figma: https://www.figma.com/file/3iOjW12ERFRDSVnpLuhQmf/landing-page-handoff?node-id=0%3A1
image

@Jarsen136
Copy link
Contributor Author

On the wallet hover popover, the total usd value of the current account should be calclate based on the real time price of KSM/BSX/xxx, according to the design.

image

image

@Jarsen136
Copy link
Contributor Author

Jarsen136 commented Oct 17, 2022

@yangwao
Copy link
Member

yangwao commented Oct 17, 2022

Other than dark mode below, I guess it's good to go and we split with upcoming #4125

Dark mode

I guess the shadow should be white.

image

image

image

image

image

@Jarsen136
Copy link
Contributor Author

Dark mode

I guess the shadow should be white.

Yup, I have fixed it.
image

@yangwao
Copy link
Member

yangwao commented Oct 17, 2022

oh found one more visual bug when in dark mode I click on connect, that white lines probably should not be there?
image

probably bolding text on the profile dropdown should be there?
cc @exezbcz

Screen.Recording.2022-10-17.at.14.40.16.mov

@Jarsen136
Copy link
Contributor Author

oh found one more visual bug when in dark mode I click on connect, that white lines probably should not be there?

✅ fixed

probably bolding text on the profile dropdown should be there? cc @exezbcz

👀

@exezbcz
Copy link
Member

exezbcz commented Oct 17, 2022

Few issues:

buttons

image
padding should be 8px - top, and bottom, font size should be 16px
different states: please refer to this image, hover is accent light, not black. Black is used only when the button is active.
image

  • Dark mode, same problem here. There should be a hover effect that looks like this:
    image

Wallet popup

  • there are two types of fonts, one is 12px font and then there is 16px (in Figma named text-button)
  • disconnect button should be made of 12px regular font with 8px top and bottom padding, 16px left and right.
  • current live version has 14px text everywhere except the button.
  • Look at the design and see where the sizes are used.
    image
  • lines that separate the content are currently to the edges, it should end 16px before the border.
    image

Explore hover

  • text should be 16px, like nav
  • bold text on hover is correct here.
  • the window should be open when hovering, not on a click. Click should redirect you to the explore page - nfts, all networks.
  • padding is 32 left right, 24 top bottom. The margin of the line is 16px - top bottom

connect button

  • lastly, there is connect button, still with fira code font, please change it to work sans.

@yangwao
Copy link
Member

yangwao commented Oct 17, 2022

  • Click should redirect you to the explore page - nfts, all networks.

we don't have this part yet :| so we can skip, I guess

@codeclimate
Copy link

codeclimate bot commented Oct 18, 2022

Code Climate has analyzed commit cfcad32 and detected 0 issues on this pull request.

View more on Code Climate.

@Jarsen136
Copy link
Contributor Author

I guess it could be reviewed again. Tell me if I miss something. @yangwao @exezbcz

@yangwao
Copy link
Member

yangwao commented Oct 18, 2022

beautiful, work like a charm!
pay 150

@yangwao
Copy link
Member

yangwao commented Oct 18, 2022

😍 Perfect, I’ve sent the payout
💵 $150 @ 37.85 USD/KSM ~ 3.963 $KSM
🧗 Caiv9TbPz68q5dC8EcHu5xKYPRnremimGzqmEejDFNpWWLG
🔗 0xba7c0c950684bc9ce66f27970208b54537934dbec5b864d3255298f130d50387

🪅 Let’s grab another issue and get rewarded!
🪄 github.com/kodadot/nft-gallery/issues

@yangwao yangwao added the paid pull-request has been paid label Oct 18, 2022
@yangwao yangwao merged commit 4abed1e into kodadot:main Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
paid pull-request has been paid
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redesign: Navbar
7 participants