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

Un-break the docs #159

Merged
merged 5 commits into from Jun 29, 2022
Merged

Un-break the docs #159

merged 5 commits into from Jun 29, 2022

Conversation

benborgers
Copy link
Collaborator

@benborgers benborgers commented Jun 28, 2022

What Broke

In #155, I merged this change to _app.tsx:

if (router.pathname.startsWith("docs")) {
return <DocsPage {...props} />;
}

This should’ve been /docs instead of docs, which meant that the <DocsPage /> layout didn’t render when it was supposed to. DocsPage includes the sidebar, mobile nav, and GlowProvider, which caused the missing sidebar, mobile nav not working, and minting not working.

Before merging the new NFT detail page I wasn’t careful enough about check the docs part of the site, so I didn’t notice that the docs were broken.

Fix

  • Replaced docs with /docs.
  • Moved the mobile nav to <Header />, so every time the mobile nav menu button is rendered in the header there’s also an actual mobile nav.

Test Checklist

Tested using https://nftoken-git-fix-revert.luma-dev.com

  • Splash page looks good in desktop and mobile.
  • “Learn more” and “Create an NFT” links work.
  • Docs logo goes to Overview.
  • GitHub link works.
  • Forward and back buttons on each page go to the correct place.
  • Glow and Twitter buttons in footer work.
  • Sidebar and header show on docs pages and are position: sticky when scrolling.
  • Docs look good on mobile.
  • Mobile nav works on docs and on NFT detail page.
  • Mobile nav works even when scrolled down the page.
  • Mobile nav closes when a link is clicked.
  • Page is not scrollable while mobile nav is open.
  • Mobile nav links work.
  • Connect/disconnect wallet works on Live Minting Demo, and so does filling out the form and clicking Create NFT.
  • Live Minting Demo looks good on mobile.
  • Code blocks are legible and can be copied via button in top right corner.
  • NFT detail page has header whose mobile nav can be expanded, but no sidebar on desktop.
  • NFT detail page’s mobile nav works when opened while scrolled partially down the page.
  • NFT detail page looks good on mobile.
  • Copying Solana addresses on NFT detail page works and causes a success toast to appear.
  • Clicking logo on NFT detail page goes to Overview.
  • Clicking links in mobile nav on NFT detail page works.
  • All pages look good when browser font size is increased (in settings, not cmd+/cmd-).
  • All pages look good in dark mode.
  • All pages look good in Safari.
  • All pages look good in Firefox.

Let me know if there’s anything else I should add to this checklist!

Outstanding Problems

  • I can’t connect wallet for the Live Minting Demo on iOS Safari. Is it a problem with it being a Vercel staging URL?

@vercel
Copy link

vercel bot commented Jun 28, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
nftoken ✅ Ready (Inspect) Visit Preview Jun 28, 2022 at 10:27PM (UTC)

Copy link
Collaborator

@vpontis vpontis left a comment

Choose a reason for hiding this comment

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

Nice! And comprehensive checklist. I think for future PRs you can run thru the main things, you don't need such a long checklist (like you don't need to check the Twitter button every PR).

@benborgers
Copy link
Collaborator Author

Sounds good, thanks!

Do you know why I’m not able to connect to Glow in iOS Safari?

@vpontis
Copy link
Collaborator

vpontis commented Jun 29, 2022

Sounds good, thanks!

Do you know why I’m not able to connect to Glow in iOS Safari?

I think it may be due to the url length. I can look into it -- want to make a task and assign it to me?

Not a blocker for merging since I think it'll work at NFToken.so

@vpontis vpontis merged commit 1dad212 into master Jun 29, 2022
@vpontis vpontis deleted the fix-revert branch June 29, 2022 04:26
@vpontis
Copy link
Collaborator

vpontis commented Jun 29, 2022

Merging this since I think we want to have less time w/ merge conflicts.

@vpontis
Copy link
Collaborator

vpontis commented Jun 29, 2022

Oh signing in doesn't work since we say the domain is nftoken.so but it's actually the luma-dev domain. I can fix that in the future.

@benborgers
Copy link
Collaborator Author

Ahh that makes sense! Out of curiosity, where do we specify the domain?

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

2 participants