-
Notifications
You must be signed in to change notification settings - Fork 0
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
UI: review usage of number of slots #155
Comments
Totes agree. I will fix the Footer, and the ACUS-only Hotel page (created separate issues before seeing this one). And, really, we have short code in settings so it would be much clearer to switch on that being acus or acnw than the slots == 8 ugliness too. |
Been plugging through. In several cases moved to settings (other issues), moved in the database the magic numbers of events (No Game / Any Game) to match ACNWs. Where else I used the configuration.abbr === 'acnw' or configuration.abbr === 'acus' instead. |
Try and bring ACNW and ACUS together through: 1. Configuration values 2. Added configuration settings: Copyright holder in footer - config.copyright (string) Used to set range on min and max player in creating games - config.playerMin (positive integer) - config.playerMax (positive integer) - config.minPlayersFloor (positive integer) - config.minPlayersCeiling (positive integer) - config.maxPlayersFloor (positive integer) - config.maxPlayersCeiling (positive integer) Used by ACUS on its hotel page to allow people to book in its block of rooms: - config.hotelBookingCode (string) - config.hotelBookingUrl (string) - config.hotelBookingLastdate (date) Resolves: #155, #158, #160, #161 See also: #159
About the most heinous piece left is packages/amber/utils/slotTimes.tsx its slot Configuration ... And putting that in config ... is doable, but going to look ugly, as each slot would need a start and end day, hour, (optionally) minutes offset. |
N.B. Vercel preview deployment fails by the look of it because the configurations are mandatory. 'd Be nice to do something that makes bunches of them optionally (as only either site uses them) with a type-specific default (0, 1/1/1970, '', etcetera). It needs the following config settings (also in the commit message):
|
Instead of sticking configuration.abbr in local variables of acnw and acus, which are essentially inverted Boolean flags, check against its values of 'acus' or 'acnw' directly. It's simpler, clearer, and allows for adding a third (even if only synthetic) site. Resolves: #155, #158, #160, #161 See also: #159
The zod schema can support this, but without separate US and NW types, the benefits of the type enforcement become much weaker. I'll pull down you branch and take a look at it. |
Yeah, I was looking at core and ACUS/ACNW-specific types or sub schemas to pull in. Not too important. Not too urgent. Again, I'm the only one messing with settings, so having cruft there is not a big deal. |
Need to also delete apps/acnw/config.ts, it's obsolete and conflicts with the new type. We also have to remember to create the new required values with the existing UIs for both installations before deploying these changes. Other than that, this looks a solid improvement. |
I'll delete apps/acnw/config.ts and commit to this branch. I don't know if I have admin access to ACNW, which is why I noted it here. |
Nope, no admin access. Obviously, with database access, I can add that. Or you can. Do you want to add the new config values, or do you want me to? I can create another admin account if you prefer (I know I do). |
Registered an account with [email protected] |
We have a number of places where we switch based on the number of slots to differentiate between acnw & acus. These should be reviewed and moved into config settings as appropriate.
The text was updated successfully, but these errors were encountered: