Merged
Conversation
Add aria role to div containing a click-event. Add enter keydown.
bg-color didn't render properly.
wip. Add conditional rendering to Header component. Temporary remove darkmode and notification icons from mobile header.
Kattenelvis
reviewed
Feb 13, 2026
| <button | ||
| on:click={() => functions[i]()} | ||
| class="bg-white w-full hover:bg-gray-300 active:bg-gray-400 dark:bg-slate-700 dark:hover:bg-slate-800 dark:active:bg-slate-900 py-2 px-4 hover:cursor-pointer transition-all whitespace-nowrap" | ||
| class="bg-white w-full hover:bg-gray-300 active:bg-gray-400 dark:bg-slate-700 dark:hover:bg-slate-800 dark:active:bg-slate-900 p-2 px-5 flex justify-between text-left hover:cursor-pointer transition-all" |
Owner
There was a problem hiding this comment.
Why justify-between when there's only a single element? Perhaps for centering, but you used text-left too?
| <Loader bind:loading> | ||
| <div | ||
| class={`sticky ${results ? 'md:-top-[1rem]' : 'top-[7.6rem] md:top-[5.5rem]'}`} | ||
| class={`sticky ${results ? 'md:-top-[1rem]' : 'top-0 md:top-[5.5rem]'}`} |
Owner
There was a problem hiding this comment.
And this doens't lead to issues in Date Poll when scrolling?
|
|
||
| <header | ||
| class="sticky top-0 z-50 md:flex justify-between flex-row items-center p-1.5 px-3 bg-white shadow select-none dark:bg-darkobject" | ||
| class="md:sticky md:top-0 fixed bottom-0 w-full z-50 md:flex justify-between flex-row items-center p-1.5 px-3 bg-white select-none dark:bg-darkobject shadow-[0_1px_5px_rgba(0,0,0,0.15)] md:shadow" |
| <div class="mr-5 flex gap-4 items-center"> | ||
| <button | ||
| class="dark:text-darkmodeText cursor-pointer pl-2" | ||
| title={`Enable ${$darkModeStore ? 'lightmode' : 'darkmode'}`} |
Owner
There was a problem hiding this comment.
do we want aria-label aswell for accessibility?
| {:else} | ||
| <Fa icon={faMoon} size={'1.3x'} /> | ||
| {/if} | ||
| {#if !$isMobile} |
Owner
There was a problem hiding this comment.
Why do both have such similar code? What about moving this conditional further down and reduce code duplication?
|
|
||
| @media (max-width: 768px) { | ||
| .active-icon::after { | ||
| top: -2.2rem; |
Owner
There was a problem hiding this comment.
Kinda ugly hard-coded 2.2rem, might not be good
| import { notifications as notificationLimit } from '$lib/Generic/APILimits.json'; | ||
| import { darkModeStore } from '$lib/Generic/DarkMode'; | ||
| import { ErrorHandlerStore } from '$lib/Generic/ErrorHandlerStore'; | ||
| import { N } from 'ethers'; |
Owner
There was a problem hiding this comment.
Might clash with what the blockchain dev is doing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Moved header to bottom on mobile.
I removed the darkmode/lightmode button and notification button since it got pretty crowded.
Need to discuss was links to keep/remove/add.