Conversation
|
Oh nice! It's going to take me a bit to review the code and I might have some code style or design feedback before it's ready to merge, but this is a great feature and I'm excited to get this in. In the meantime can you please fix the whitespace stuff so the diff is just code changes? (There's some whitespace/formatting clean up I've been meaning to do for a while so your version of the whitespace might even be "right", but having it mixed in with significant code changes is distracting) |
|
Whitespace fixed - and feedback is very welcome :) |
|
The custom form doesn't work at all with javascript disabled. At the very least we should gracefully fall back to a message that asks the user to enable javascript to use the form, but ideally I'd like for it to be usable (if less magical) with no JS at all. I think the right way to do that is to add a POST endpoint in the server that redirects to the appropriate full path. |
|
Hey could we actually separate the json api part into its own PR? That's relatively self-contained and we could go ahead and get that merged while hashing out the exact design of the custom pronouns form. |
|
OK, I've created another pr with just the json stuff, but I'm not sure what to do about the json commits in this one. Re: JavaScript disabled. Just to confirm, by "doesn't work", do you mean it's actually broken or is disabled completely? My original plan for this was that when js is turned off, the form is hidden - so the site looks/functions exactly as it does at the moment. But yeah, having a non-js fallback would be great. The only thing I'm unsure of is what to do about the subject pronoun in the 3rd and 5th examples when we can't update them as the user types. At the moment it just defaults to a random pronoun which isn't really right. I guess we could have it read 'X brought ......... frisbee', but that doesn't seem great either. |
Yeah it's an annoying edge case in git. I believe that since you cherry-picked them into the other PR if we merge both it'll just only apply those diffs once and do what we want. (And, hopefully once the other PR is merged github will stop showing those lines in the diff). If it messes up we can fix it manually, nbd
Oh, just disabled completely.
What about " brought ... frisbee"? That makes it clearer to the reader what is supposed to go there, even if it's a little bit ugly. There should also be a message asking the user to turn on JS for full functionality. |
|
The form now works with JS disabled :) |
WOOP!
This adds a custom pronoun form on the home page and the 404 page. It also adds a little JSON API (as discussed in #29) which is used to match pronouns already in the database as the form is filled out. On the 404 page, the form is pre-populated with whatever pronouns are already in the URL.
I'm don't have a huge amount of Clojure experience so let me know if there are any problems with this - the stuff for capitalising strings/inputs/spans in particular seems dodgy but I'm not sure how to make it better.
Also apparently my editor made a bunch of whitespace changes because computers. I can fix that if needed.