-
Notifications
You must be signed in to change notification settings - Fork 277
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
Filter entries by language type in onefetch.dev #1227
Conversation
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.
Looks good! Just some nitpicks mostly.
The one big thing that I'm wondering is if if makes sense to have "programming" and "markup" be checked by default. This could help hint to what onefetch
's defaults are. Perhaps a "reset to default" button could help illustrate this? Though that's a bit of overkill for 4 checkboxes 😆
I see you've written the checkboxes to be dynamic by utilizing a Set<string>
instead of hard-coding. But, to set default checked values, I think you can't get around doing a little bit of hard-coding. But, given how infrequently the language types are likely to change, I think it's OK to do a bit of hard-coding 🙂
docs/vercel/src/Index.svelte
Outdated
<div class="title"> | ||
<h3>Languages <small>({$filteredLanguages.length})</small></h3> | ||
<button class="filter-button" on:click={() => (menuOpen = !menuOpen)} | ||
>Filter by type</button> | ||
</div> | ||
|
||
<div class:show={!menuOpen} class="checkbox-group"> | ||
{#each languageTypes as type} | ||
<label for={type}> | ||
<input | ||
id={type} | ||
type="checkbox" | ||
value={type} | ||
bind:group={$filter.checkboxes} /> | ||
{type} | ||
</label> | ||
{/each} |
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.
TBH I wonder if the filters really need to be shown/hidden, since they don't take up much space.
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 now shown by default.
docs/vercel/src/Index.svelte
Outdated
const filter = writable({ | ||
checkboxes: [] as string[], | ||
}); | ||
|
||
const filteredLanguages = derived(filter, ($filter) => { | ||
let filtered: Language[] = languages; | ||
if ($filter.checkboxes.length > 0) { | ||
filtered = filtered.filter((d: Language) => | ||
$filter.checkboxes.includes(d.type) | ||
); | ||
} | ||
return filtered; | ||
}); |
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.
I'm more of a Vue person than a Svelte person, but in Vue I would've probably bound the checkbox values to a { [key: string]: boolean }
instead of a string[]
. Then the filtered languages could be languages.filter(({ type }) => $filter.checkboxes[type])
.
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.
If I do this I won't be able to use the bind:group
on the input element easily. I'm also not familiar with Svelte.
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.
Ah, OK, no worries. This definitely isn't a blocker for me.
Hi @spenserblack, thanks a lot for the review 👍
I see your point but I'm just not sure if we should restrict the default output of |
related to #1208
Implementing a simple filtering option to make language type information more accessible to users.
@spenserblack, I'm not particularly skilled in UI/UX (or Svelte) 😅 . Feel free to be honest and share your thoughts 🙏