-
Notifications
You must be signed in to change notification settings - Fork 37
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
Use shadcn/ui
as the primary component library for the frontend.
#104
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.
My main problem with this is the regression on the modal behaviour. I think the best solution is to have an object like the modal controller, though it could probably be simplified (but maybe best to try to reuse it for now? unless unconvenient)
Reviewing this made me think we should probably improve the readabilty of the modals (increase size, use better contrast), and that we should implement auto-drawing (e.g. when you detect is your turn automatically send the draw tx) — I'll make issues for both these things.
packages/webapp/src/pages/_app.tsx
Outdated
@@ -37,6 +38,13 @@ const MyApp: AppType = ({ Component, pageProps }) => { | |||
<Head> | |||
<title>0xFable</title> | |||
<link rel="shortcut icon" href="/favicon.png" /> | |||
<link | |||
rel="preload" | |||
href="/fonts/BluuNext-Bold.otf" |
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.
href="/fonts/BluuNext-Bold.otf" | |
href="/font/BluuNext-Bold.otf" |
Was wondering what was causing this warning: "The resource http://localhost:3000/fonts/BluuNext-Bold.otf was preloaded using link preload but not used within a few seconds from the window's load event. Please make sure it has an appropriate as
value and it is preloaded intentionally."
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.
Well RIP. This was obviously an issue, but it does not get rid of the warning, at least in dev mode. Looking at the network tab, you can see the font is indeed loaded twice.
Any idea what's going on?
TODO: test in prod mode
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.
The font loads twice because it's once loaded by _app.tsx
and then once again by globals.css
.
Looking into how to reduce to this to one.
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.
Yup, my understanding is the point of the preload link is to load it only once, before the CSS loads so it does not need to load again.
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.
This doesn't seem fixed, so I suggest we remove it until we can figure out how to make the preload work correctly.
} | ||
} | ||
|
||
// ================================================================================================= | ||
|
||
export type LoadingModalProps = { | ||
ctrl?: ModalController |
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.
So I was afraid of this when I saw you removed the controller, but that thing existed for a reason!
The problem right now is that all modals have undifferentiated behaviour, meaning you can all close them by clicking outside of them. For instance, try creating a game, then click outside the modal while it is loading, which could easily happen by accident ... context is lost. And re-clicking "Create Game" does not restore it.
We could fix that (i.e. reset the context when reopening the modal, by shifting the state around), but the point is the user should really not be able to close the modal at that point.
If memory serves, some modals were also "closeable" but not "surround closeable", meaning you could quit them with the cross, with escape (and always irrc with a custom button) — this is a good config for error messages that you don't want to disappear when the user clicks outside (which he might do for window focus reason, etc)>
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.
That being said, we probably don't need something like the controller absolutely, but then we need more parameters to pass into the modal component and corresponding useState
to be able to toggle them.
The idea behind the controller was:
- to encapsulate all that state away and just keep a single object.
- you need to set that state both from calls defined outside the component (in the parent, e.g. in a page), and also inside the component, and that state should stay in sync (e.g. when you press the cross to close the modal, that should close it but also sync the
useState
that says whether it should be displayed.
It also had the distinction between "hidden" (DOM retained) and "closed" (DOM removed), but I'm not sure how significant that is in the codebase (needs a closer look). The distinction is relevant whenever you put stuff with React state inside the modal!
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.
To account for modal specific behaviour, i'm using the onEscapeKeyDown
and onInteractOutside
props exposed by the DialogContent
element, both behaviours that are controlled by specific conditions.
This recreates the closeable
and surroundCloseable
mechanisms that were earlier defined.
const isGameCreator = store.useIsGameCreator() | ||
const gameStatus = store.useGameStatus() | ||
|
||
const preventSurroundClick = gameStatus >= GameStatus.CREATED && loading; // created and loading | ||
|
||
const isSurroundCloseable = loading !== null || (gameStatus >= GameStatus.CREATED) // state is loading or game is in created 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.
This should be reversed if I'm not mistaken. We only want to allow closing by clicking outside if we haven't created the game yet, or if we're loading while waiting for it to be created.
So loading == null || gameStatus < GameStatus.CREATED
You also inverted the condition in the handlers has well (i.e. you prevent it from closing if "closeable" is true) so the behaviour ends up as expected, but that makes the variable name wrong
onInteractOutside={(e) => loading !== null ? e.preventDefault() : null} | ||
// prevent modal from closing if esc key is pressed and loading is populated | ||
onEscapeKeyDown={(e) => loading !== null ? e.preventDefault() : null} | ||
> |
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 abstract those inside the DialogContent
with closeable
and surroundCloseable
properties that apply these behaviours.
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 keying on loading
is not sufficient here, we want to forbid closing the modal if we've joined the game status >= GameStatus.JOINED
).
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.
Finally, both for this and the create game modal, the cross at the top right of the modal is still displayed and allows closing the model — we want to hide it when the modal shouldn't be closeable.
packages/webapp/src/pages/_app.tsx
Outdated
@@ -37,6 +38,13 @@ const MyApp: AppType = ({ Component, pageProps }) => { | |||
<Head> | |||
<title>0xFable</title> | |||
<link rel="shortcut icon" href="/favicon.png" /> | |||
<link | |||
rel="preload" | |||
href="/fonts/BluuNext-Bold.otf" |
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.
This doesn't seem fixed, so I suggest we remove it until we can figure out how to make the preload work correctly.
This error is displayed: Sadly, this is mangled by my I disabled the filtering (by commenting the line in |
Another feature that got lost is that the code would force opening the correct modal depending on your in-game state, e.g. if you've joined a game or created a game it would open the corresponding modal (because you can't be joining multiple games at once, trying to create a game while being joined in another would fail). |
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.
(ignore I'm just testing branch protection settings in Github)
if you have joined a game, and refresh the tab and try to create another one, it shows the |
This only works once the game is started. If you have created a game in reload, the createGameModal doesn't pop up. Similarly, if you have joined a game but the game hasn't started yet, the joinGameModal doesn't pop up. These were behaviours that were present in the older versions. You can lookup there, there was a value being computed that was passed to the modals to determine whether they'd be initially open or not (I think it might have been a call to the controller in |
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.
Seems like the DOM error comes from the joinGameModal in particular.
Checklist before merge:
[ ] Get rid of the DOM error
[ ] Remove the preloaded font
[ ] Make sure the appropriate modal is d when we created / joined a game (use old version as ref for the condition) and the page is reloaded
[ ] Make sure we're not needlessly removing styling on the close button
<span className="sr-only">Close</span> | ||
</DialogPrimitive.Close> | ||
{canCloseExternally && ( | ||
<DialogPrimitive.Close className="absolute right-4 top-4"> |
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.
Is it okay to get rid of all the tailwind classnames here, like the focus and hover stuff?
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.
this comes in directly from the docs: https://ui.shadcn.com/docs/components/dialog
it's fairly comprehensive in its usage of tailwind features, what would you like removed/abstracted?
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.
The comment preview is shitty, but go to the file & look at the diff, you're replacing a very long list of tailwind class names for DialogPrimitive.Close
with just two, and I was wondering why / if it was intentional.
const DialogContent = React.forwardRef< | ||
React.ElementRef<typeof DialogPrimitive.Content>, | ||
CustomDialogContentProps | ||
>(({ className, children, canCloseExternally = true, ...props }, ref) => ( |
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.
Okay to use a single property instead of closeable / surroundCloseable.
The point of that distinction is that you want to have some modals that the user can close but shouldn't close carelessly by clicking outside, like some error displays. I don't think we were using that capability anywhere, so a single property will do for now. Also we can always make modal closeable with an explicit "okay" button which is probably better anyway.
It seems to be coming from here: Let's investigate this later, I'll open an issue for it. |
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 golden, that was a lot of work.
Let's go 🚢 🚢
Refers to #99.
daisyui
has been removed from the necessary package files.New components have been copied from the shadcn docs (button, navigation-menu, dialog, input).
Only necessary
radix-ui
components have been installed as instructed by the docs. Upon successful review, will remove themodal.tsx
andModalController
files, since those aren't being used any longer.Added new font and new spinner svg as well. ✨