-
Notifications
You must be signed in to change notification settings - Fork 65
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: Mobile OnThisPage #165
Conversation
🦋 Changeset detectedLatest commit: a437f63 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
> | ||
</li> | ||
{#each details.sections as { title, slug }} | ||
<button class="heading" on:click={() => (mobile_menu_open = !mobile_menu_open)}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should toggle aria-expanded="true"
or aria-expanded="false"
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
on:click={() => dispatch('select')}>{title}</a | ||
href="{base}/docs/{details.slug}" | ||
class:active={hash === ''} | ||
on:click={() => dispatch('select')}>{details.title}</a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be nice to add aria-current="page"
here for the currently active hash https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-current
then we could even target that instead of .active
to sync a11y state and visual state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(also on the iterated sections below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
* { | ||
box-sizing: border-box; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need this? I'd expect this to be set globally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops yeah, left it while tryna debug something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
</span> | ||
</button> | ||
|
||
{#if (browser && !$is_mobile) || ($is_mobile && mobile_menu_open)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we checking browser
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise the styles flicker badly on page load, so browser is required
> | ||
</li> | ||
{#each details.sections as { title, slug }} | ||
<button class="heading" on:click={() => (mobile_menu_open = !mobile_menu_open)}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this button only do something when on a mobile screen?
if so, we should only render a button when we're on a mobile screen. otherwise we'll have a control that does nothing at some screen widths
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On on mobile screen, yep!
Is rendering separate elements necessary? That would complicate things somewhat, and I'd rather not do it, unless its a deal breaker for screen readers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not great, since you have an interactive control that doesn't do anything. But as a screen reader user you might assume that it is doing something, it's just not being announced properly
Can't we just use <svelte:element>
and toggle between button and div?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
list-style: none !important; | ||
margin-left: 0 !important; | ||
} | ||
|
||
li { | ||
margin: 0.2rem !important; | ||
} | ||
|
||
li::before { | ||
content: none !important; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're using a lot of !important
here - is that really necessary? what styles are we trying to override?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its inside class="text"
. !important is needed to override those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, I see -- it's because we're moving this inside the docs content. still feels like a code smell, but maybe not urgent to resolve as part of this work.
it might be a nice refactor in the future to use something like CSS layers or :where
in the global styles to reduce specificity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened this PR for your review #167
Related sveltejs/svelte#8782