-
Notifications
You must be signed in to change notification settings - Fork 3
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
Upgrade prettier, add some ui elements, add the Register page. #670
Conversation
information_circle: InformationCircleIcon24, | ||
}; | ||
|
||
const icons20 = { |
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 need both 24 and 20? would it be worth it to cut down and only need to import one of them?
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.
(jumping in here as someone who hasn't been paying attention and who has just read some email digests)
I think it's ok to have both, because it makes sense to be able to tailor/change icon-sets for different episodes of Battlecode. But, this is conditioned on:
- The setup being something easily extensible to n episodes of Battlecode for large n.
- I think it's ok if this extensibility means defining a new dict/mapping for each episode, because that's unavoidable when each episode could be different.
- However, it should also be implementable without changing the downstream rendering code, so this looks a bit more dubious. Perhaps it could be a single mapping that maps each episode name to one of these dicts?
Separately, curious how easy it would be to use this framework to add new menu items in the future with new icons? (possibly the answer is "very easy", I didn't read the code at all)
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, 24/20 px not episode, sorry for the confusion)
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.
20/24 represents the icon size, not the episode of battlecode. We use smaller icons for buttons / form elements, and bigger icons are used for navigation.
We would use the same icon set across all episodes of battlecode.
Separately, curious how easy it would be to use this framework to add new menu items in the future with new icons? (possibly the answer is "very easy", I didn't read the code at all)
new menu items per episode can be added like this:
{episode === 'bc123' &&
<SidebarItem
iconName="whatever_icon_name"
text="Resources"
linkTo={`${linkBase}resources`}
/>
}
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, if either size gets used for different purposes, then sounds good
I'm a little wary of the extra overhead here -- if we want to add a new icon, then we'd have to import both sizes, and make sure the correct size gets used. But perhaps this is easier than I think. Most of all though, we could always come back to this later
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 fair, naming wasn't entirely clear but makes sense now
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.
LGTM pending resolution of Jerry & Nathan's comments 🙌
frontend2/src/views/Register.tsx
Outdated
console.log("logged in successfully"); | ||
} catch (error) { | ||
console.log("failure to register", error); |
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.
Consider showing a user-facing error, even just propagating the backend message up to the frontend is fine. (errors where nothing seems to happen would be annoying to user)
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.
and it's not clear to me that the error does show, especially if it's in the login method? If it does feel free to ignore me, I haven't locally tested yet
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'll do this in a separate PR, as this PR is already pretty big. In that PR I'm also planning on creating a reusable component for displaying these errors.
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.
oooo a reusable error comment, niceeee
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.
Actually for this PR I can make a super simple error message and replace it in the later PR
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.
overall looks good, do see comments though!
Adds the register page.
UI elements added / styled (these have all been tested, see the Register.tsx page):
Prettier changes:
prettier-plugin-tailwindcss
listed inprettierrc.json
Before testing, make sure to run
npm install
to get the updated packages.