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

🔧 migrate b-navbar #6382

Merged
merged 16 commits into from
Jul 6, 2023
Merged

🔧 migrate b-navbar #6382

merged 16 commits into from
Jul 6, 2023

Conversation

roiLeo
Copy link
Contributor

@roiLeo roiLeo commented Jul 4, 2023

@prury you have to check if nothing breaks in navbar (mobile, desktop, links etc..) compared to beta/canary

edit: Mobile navbar doesn't work hehe fixed 🎉

note: Didn't changed StatsDropdown component since it's not used

@roiLeo roiLeo requested a review from a team as a code owner July 4, 2023 08:01
@roiLeo roiLeo requested review from preschian and daiagi and removed request for a team July 4, 2023 08:01
@netlify
Copy link

netlify bot commented Jul 4, 2023

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit 72bd5db
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/64a6c1c07516450008f9ba43
😎 Deploy Preview https://deploy-preview-6382--koda-canary.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 configuration.

@reviewpad
Copy link
Contributor

reviewpad bot commented Jul 4, 2023

AI-Generated Summary: This pull request modifies the Navbar component in a Vue.js application. It migrates the structure of the Navigation bar from using <b-navbar> component to a more semantic HTML structure using <nav> and other related tags. Additionally, it changes the <b-navbar-item> tags in the 'CreateDropdown.vue' and 'MobileLanguageOption.vue' components to <nuxt-link> and <a> tags, respectively. The motivations for these changes are not mentioned, but likely due to improving code readability and accessibility, as '

' is an HTML5 semantic tag.

@reviewpad reviewpad bot added large Pull request is large waiting-for-review labels Jul 4, 2023
@roiLeo roiLeo marked this pull request as draft July 4, 2023 08:09
@roiLeo roiLeo marked this pull request as ready for review July 4, 2023 08:23
@roiLeo roiLeo mentioned this pull request Jul 3, 2023
37 tasks
Copy link
Contributor

@daiagi daiagi left a comment

Choose a reason for hiding this comment

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

looks good
didn't find anything that isn't working

components/Navbar.vue Outdated Show resolved Hide resolved
Co-authored-by: Luke Fishman <daiagi@gmail.com>
@prury
Copy link
Member

prury commented Jul 4, 2023

Mobile:

Javascript chain 😄 (only happened the first time i've entered using the deploy link )


When you are disconnected on mobile and try to connect, the burger menu is not closing to allow the user to connect:

connect.mp4

Language button when the wallet is disconnected on mobile:
it returns a 404 page or it refreshes and don't change the language

when those get fixed I'll do another round of testing

@prury prury added the S-changes-requested-🤞 PR is almost good to go, just some fine tunning label Jul 4, 2023
@roiLeo
Copy link
Contributor Author

roiLeo commented Jul 4, 2023

Blocked:

TypeError: can't access property "abstract", parent.$options is undefined
related with #6277

edit: found a workaround

@roiLeo roiLeo marked this pull request as draft July 4, 2023 15:35
@roiLeo roiLeo marked this pull request as ready for review July 5, 2023 11:23
@roiLeo roiLeo requested a review from prury July 5, 2023 11:23
erge branch 'main' into chore/Buefy/b-navbar
@prury
Copy link
Member

prury commented Jul 5, 2023

@roiLeo switching language on mobile changes the language but the modal won't close as soon as i click it, it keeps open.

@roiLeo
Copy link
Contributor Author

roiLeo commented Jul 5, 2023

@roiLeo switching language on mobile changes the language but the modal won't close as soon as i click it, it keeps open.

Isn't it the current behavior on beta? are you logged in?

@prury
Copy link
Member

prury commented Jul 5, 2023

@roiLeo switching language on mobile changes the language but the modal won't close as soon as i click it, it keeps open.

Isn't it the current behavior on beta? are you logged in?

i'm not logged and using mobile

beta behavior:

beta.mp4

deploy:

6382.mp4

@roiLeo
Copy link
Contributor Author

roiLeo commented Jul 5, 2023

Should be good now, thanks for reporting

@prury
Copy link
Member

prury commented Jul 5, 2023

Should be good now, thanks for reporting

ty, will do another round of testing now

@prury
Copy link
Member

prury commented Jul 5, 2023

  • scroll / search bar button issue(only happens on main page):
scroll.mp4
  • Search button opening the entire modal (beta only shows the search bar itself):
search.button.opening.the.whole.modal.mp4

found a old bug: #6395

for now i think that's it, but i wouldn't mind testing a bit more later

@roiLeo
Copy link
Contributor Author

roiLeo commented Jul 5, 2023

  • remove scroll when mobile navbar open

@roiLeo
Copy link
Contributor Author

roiLeo commented Jul 6, 2023

closes #6395, feel free to test again

@roiLeo roiLeo mentioned this pull request Jul 6, 2023
@prury
Copy link
Member

prury commented Jul 6, 2023

thanks a lot for the fixes @roiLeo

only problem i see now is this one:

search.mp4

search icon not appearing on other pages, only inside menu

@codeclimate
Copy link

codeclimate bot commented Jul 6, 2023

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

View more on Code Climate.

@roiLeo
Copy link
Contributor Author

roiLeo commented Jul 6, 2023

🚀 letsgo

@prury prury added S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked and removed S-changes-requested-🤞 PR is almost good to go, just some fine tunning labels Jul 6, 2023
@roiLeo roiLeo merged commit f39aaff into kodadot:main Jul 6, 2023
18 of 19 checks passed
This was referenced Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
large Pull request is large S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked waiting-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navbar modal won't close when clicking explore items or collections
4 participants