-
Notifications
You must be signed in to change notification settings - Fork 26
Jake's First Updates :) #151
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
base: v3
Are you sure you want to change the base?
Conversation
|
added open team sheet handler and fixed some potential bugs |
|
oh I also added a potential fix to make line breaks not disappear in the Notes section in syntax.go |
felixphew
left a comment
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.
Thanks for the contributions!
A lot of these are the kind of change that I'd want to discuss how to implement - e.g. the gender differences introduces a lot of new behaviour that I'd want to think through, the teamsheet bit will require a change to the database schema... also, some things are clearly half-done and haven't been tested - the open teamsheets support comes to mind. If I remember, I'll message you tomorrow with some information about how to get a test environment going.
The IoA / CT / galar formes, and the little clarifications for where to paste and so on, are very welcome, and it'd be great if you could send those in a separate PR or two so I can merge them. That's the trouble with submmitting a whole pile of changes like this. (Don't include the gender differences bit in the data update PR, if you make one.)
But thanks for helping out, and sorry I've been so preoccupied lately!
| template.HTMLEscape(&b, m[0]) | ||
| b.WriteString(`</span>`) | ||
| if len(m[0]) == 5 && m[0][1] == 'V' && m[0][2] == 's' { | ||
| if len(m[0]) == 5 && m[0][1] == 'V' && m[0][2] == 's' && !openTeamsheet { |
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 won't actually hide EVs and IVs - just disable highlighting them, as they'll be caught by the else on line 185. To achieve this effect, you'd want a nested if inside, not an &&.
More importantly, putting the condition here will mean the "EVs:" or "IVs:" header will still be printed - you probably want to put this test further out.
| switch p { | ||
| case "", "/": | ||
| renderPaste(w, paste, title, author, notes) | ||
| renderPaste(w, paste, title, author, notes, openTeamsheet) |
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.
There's no openTeamsheet variable in scope here - this function renders a paste after it's come back out of the database, and you never store it above. if you want to handle open teamsheets like this, you'd need to add them to the database schema. I'll need to think about whether that's what we want to do, though.
| func renderPaste(w http.ResponseWriter, text, title, author, notes []byte) { | ||
| func renderPaste(w http.ResponseWriter, text, title, author, notes []byte, openTeamsheet) { | ||
| sets := bytes.Split(text, []byte("\r\n\r\n")) | ||
| title = strings.ReplaceAll(title, "\n", "<br>") |
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 will result in CRLF line endings translating to two <br>s - not ideal. Title shouldn't ever be multiline anyway.
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.
wow... I very much meant for these to be notes lol
| } | ||
|
|
||
| if len(m[6]) != 0 { | ||
| if p, ok := pokemonData[string(m[6])]; ok { |
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.
Not keen on just defaulting to male sprites if gender's unspecified - we can be a bit more creative than that.
|
Couldn't we use the Home Sprites database ? I may have missed something up. |
IoA and CT updates
Galar Formes
Gender and Shiny art