Skip to content

Conversation

@PuruVJ
Copy link
Collaborator

@PuruVJ PuruVJ commented May 19, 2023

No description provided.

@benmccann
Copy link
Member

@PuruVJ it looks like there's quite a few lint errors probably from being based on an older version of the version-4 branch. These have all been fixed in the version-4 branch now

@benmccann
Copy link
Member

I was going to test this out on mobile, but it looks like the deployment is failing, so I can't at the moment:

Error: Missing "./components/docs" specifier in "@sveltejs/site-kit" package

@PuruVJ
Copy link
Collaborator Author

PuruVJ commented May 23, 2023

Still in the middle, working on the content restructuring ones first. I'll let you know once this is done

@Rich-Harris
Copy link
Member

We shouldn't have a setNavTitle function — it's brittle, and doesn't work with SSR. If we want to show the current page in the nav (do we? what's the argument for doing so?) then it should be done with $page.data.nav_title or something

"check:format": "prettier --check . --ignore-path .gitignore --plugin-search-dir=."
},
"dependencies": {
"@jridgewell/sourcemap-codec": "^1.4.15",
Copy link
Member

Choose a reason for hiding this comment

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

you shouldn't need to do this. update @sveltejs/kit instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Didn't work. Error is still there even after maxing out all versions and removing sourcemap-codec

Comment on lines 45 to 47
resolve: {
dedupe: ['@jridgewell/sourcemap-codec']
},
Copy link
Member

Choose a reason for hiding this comment

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

you shouldn't need to do this. update @sveltejs/kit instead

Suggested change
resolve: {
dedupe: ['@jridgewell/sourcemap-codec']
},

@PuruVJ
Copy link
Collaborator Author

PuruVJ commented May 30, 2023

@Rich-Harris is the function call the one tripping up HMR and SSR? Or is it just using a Svelte Store in general? I pushed a change which directly writes to the store, lemme know if its still ineffective. I'll go with the $page.data method then

@Rich-Harris
Copy link
Member

Writing to a store from a page means that in SSR, the nav has already been rendered, so you'll see a flash of un-SSR'd content. Returning a nav_title or whatever from the page load function means you can do this anywhere in the tree:

<p>{$page.data.nav_title ? ''}</p>

When I say it's brittle what I mean is that you need to remember to update the store (or call the function, whatever) on every route. If a new route is added in future and we forget to do that, the nav will just continue to show the result of the most recent set/call, which is much worse than it being blank.

If we make the property required...

// src/app.d.ts
declare global {
  namespace App {
    interface PageData {
      nav_title: string;
    }
  }
}

export {};

...then in theory we'll even get type safety (i.e. a page load function will be invalid if neither it nor a parent load function returns a nav_title property), though there's currently an issue with that: sveltejs/kit#9799

@PuruVJ
Copy link
Collaborator Author

PuruVJ commented May 30, 2023

Gotcha!

@Rich-Harris
Copy link
Member

Ok, I've figured out how to get this running locally, so I can actually try it out.

Can we move the bar to the top, and fix it to the nav (i.e. it's visible when the nav is visible, but otherwise slides out of the way)? It takes too much real estate to be a permanent presence.

I'd almost be inclined to get rid of the 'on this page' button on mobile. It provides nice visual balance on desktop and gives you a sense of progress as you're scrolling down, but it's probably overrated as a navigation device. react.dev just hides it altogether on mobile, for example.

To be honest I'm not sold on the subtitle on the nav, the tiny type feels rather cluttered, and I don't feel like it's necessary.

@PuruVJ PuruVJ marked this pull request as ready for review June 12, 2023 16:28
@PuruVJ PuruVJ merged commit aa5bb4a into sites Jun 12, 2023
@PuruVJ PuruVJ deleted the feat/better-docs-nav branch June 12, 2023 16:29
@gtm-nayan gtm-nayan mentioned this pull request Jun 17, 2023
5 tasks
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.

4 participants