Add font menu previews and virtual scrolling #650
Conversation
Deploying with
|
| Latest commit: |
13e645b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bec95788.graphite-master.pages.dev |
| Branch Preview URL: | https://font-prieviews.graphite-master.pages.dev |
|
I'm heavily refactoring these files right now so hopefully this won't be too hard to port over the changes once I'm ready to merge mine. |
|
Oh right sorry. |
|
Nothing to apologize for! Just giving you an update on the status. This looks like an exciting feature! |
0HyperCube
force-pushed
the
font-prieviews
branch
from
9505560 to
b005ea0
Compare
|
For the record, but not relevant in this PR: My plan for the design of the FontInput widget will involve using a single component that encapsulates two DropdownInputs. (And from a UI/UX standpoint, we'll tailor it more closely to provide useful font-related things, like a search box to filter the list and a button to upload fonts). By making the one component use the two DropdownInput components as children, we can avoid duplicating code used by the DropdownInput component and avoid needing to check Just mentioning this for future reference. |
|
It would also be a cool feature to display the font weight as a preview. |
0HyperCube
force-pushed
the
font-prieviews
branch
from
b005ea0 to
7c32f93
Compare
Keavon
force-pushed
the
master
branch
2 times, most recently
from
a86fd02 to
fd95326
Compare
|
If you get a chance to fix the merge conflicts, I think this should be pretty quick to get merged since it has a great UX/behavior already. (I'll just skim over the code but there's probably nothing major to change.) |
frontend/src/io-managers/fonts.ts
Outdated
| return { | ||
| state: readonly(state) as typeof state, | ||
| fontNames, | ||
| getFontStyles, | ||
| getFontFileUrl, | ||
| }; |
There was a problem hiding this comment.
I may have misdirected you about this being an I/O manager. The format for all I/O managers is that they must return from their constructor either nothing or a single destructor function. Not an object with several functions. I don't think utility-functions is the correct folder for this either, though, because those are all stateless single-purpose functions. Feel free to read frontend/src/README.md for a bit more detail on each of these types. Do you think this could maybe be restructured to fit the conventions for any of the others? I assume you can't make this stateless to work with utility-functions. Maybe the getter functions could be changed into entries for the state object and this could become a state provider again? Happy to discuss this further, I don't mean to misdirect you again.
There was a problem hiding this comment.
Why should they become entries in a state object? This would be additional complexity for no gain.
There was a problem hiding this comment.
I'm just trying to figure out how we can make it match the code style of the other files to avoid confusing inconsistency. My idea about state was that, perhaps instead of exposing these getter functions, we can instead just bind the current value to the state object so users can replace their getter function calls with reading the state values? I don't know if that is the right approach here or not, but just an idea. If you'd like to talk more about this on Discord, we can probably settle it out real quick, I just want to make sure we're using our frontend architecture in a not-inconsistent way.
|
Done with this round of code review comments. I'll look more closely at |
|
Hmm... capture_2_.mp4 |
|
Awesome! There's only one very minor polishing behavior I'd like to eventually fix, but it's probably worth leaving for a later date. When you scroll, once you reach the bottom of a loaded chunk, it bottoms out and doesn't let you keep scrolling with the mouse wheel until you move your cursor by a pixel. Kind of strange. But I'm happy to merge this now unless you prefer to take a poke at that real quick. |
That is quite odd - this does not happen on firefox but I can reproduce it on chromium. The loaded chunk is just the 21 visible entries (you can see 21 x 20px entries in the 400px height if the scroll is not offset by a multiple of 20) so this doesn't happen when you reach the end of that. |
* Keyboard menu navigation * Fix dropdown keyboard navigation * Fix merge error * Some code review * Interactive dropdowns * Query by data attr not class name * Add locking behaviour * Add font prieviews * Remove blank line in css * Use default for interactive in struct * Use menulist for fontinput * Polish * Rename state -> manager * Code review * Cleanup fontinput * More cleanup * Make fonts.ts an empty state * Fix regression Co-authored-by: Keavon Chambers <[email protected]>
* Keyboard menu navigation * Fix dropdown keyboard navigation * Fix merge error * Some code review * Interactive dropdowns * Query by data attr not class name * Add locking behaviour * Add font prieviews * Remove blank line in css * Use default for interactive in struct * Use menulist for fontinput * Polish * Rename state -> manager * Code review * Cleanup fontinput * More cleanup * Make fonts.ts an empty state * Fix regression Co-authored-by: Keavon Chambers <[email protected]>
Closes #596
