Language Form Final #1064
Language Form Final #1064
Conversation
There was a problem hiding this comment.
Works well and does what it needs to do, though there are some places where it could use minor improvements.
I left some comments already for potential changes in the code, but one minor functional thing I noticed is that when we have several issues that the language form picks up, if I fix one and move onto the other, there's the potential to show errors when visually everything looks fine.
The solution would probably be to reset the values for each input when activeIssue changes.
| const [useBCP47, setUseBCP47] = useState(false) // If user wants to input custom BCP47 rule - deafault to false as most fauclty will just select language | ||
| const [textInputBCP47, setTextInputBCP47] = useState("") // The text input for checking BCP47 | ||
|
|
||
| const [removeLanguage, setRemoveLanguage] = useState(false) // Checkbox for if we want to remove the language tag or not | ||
| const [language, setLanguage] = useState("") // The actual language or language code that's being used | ||
| const [inputErrors, setInputErrors] = useState([]) // Populates if user has errors with inputs (BCP47 validity or somehow choosing an invalid language) | ||
| const [options, setOptions] = useState([]); // Options used for combobox | ||
|
|
||
| const [isHtml, setIsHtml] = useState(false); // Checks if it is a html tag |
There was a problem hiding this comment.
Given that all these are set at the Language Form file, you could probably just remove these comments as they don't add much that the user can't figure out by reading the const values.
| // Only check for errors if and only if we are STILL using the lang tag | ||
| if(!removeLanguage){ | ||
| if(useBCP47){ // If we are using BCP47 textbox we must check for valid BCP47 | ||
| if(!validBCP47()){ | ||
| tempErrors.push({text: t('form.language.error.invalidBCP'), type: "error"}) | ||
| } | ||
| } |
There was a problem hiding this comment.
Could potentially just turn this into:
if(useBCP47 && !validBCP47)
tempErrors.push...
and just deselect removeLanguage when the user selects the useBCP47 checkbox. Feels cleaner than 2 nested if statements.
|
|
||
|
|
||
| const handleLangChange = (id, value) => { | ||
| // Set the new language selected by the user here |
There was a problem hiding this comment.
This comment feels unnecessary with the context of the set function and handleLangChange in the function name.
| else{ // We are not removing the language | ||
| if(useBCP47){ // If we are change | ||
| element = Html.setAttribute(element, "lang", textInputBCP47) | ||
| } |
There was a problem hiding this comment.
Make this an else if(useBCP47) since that's essentially what's already happening here.
Also here and throughout the file, move the comments above the line they reference (add spacing if that helps) so the comments are consistent throughout. I see some in-line comments and some above, so it would help to have them all above the lines referenced.
| const html = Html.getIssueHtml(activeIssue) | ||
| let element = Html.toElement(html) | ||
|
|
||
| // Case 1: We are removing the lang tag |
There was a problem hiding this comment.
Also unnecessary. If the user can discern this from the variable name, you're probably good.
| } | ||
|
|
||
|
|
||
| // On every update of our activie issue, we want to set it to the language it's using (even if it's not an existing language) |
| } | ||
|
|
||
|
|
||
| // O(1) or however many languages there are when we first load in |
There was a problem hiding this comment.
Maybe just mention this is the initial constructor for the form options, if anything. It's neat but the runtime doesn't help the user understand what we're looking at.
There was a problem hiding this comment.
Lots of good changes! I did like that now the language selected dropdown gets reset if we move from one LanguageForm issue to another. I did notice a couple strange things with the other form options though:
- If you choose to remove the language, save, and move to another issue, the issue is now removed from the list, so you can't go back to it and it doesn't even show with the
fixed issuesfilter turned on. Not sure what's going on there, but the user should be able to easily move back to the issues fixed. - If you input a value for BCP47, it doesn't persist when you go back and forth between issues. This should be the behavior, in case the user decides to change it later on, and needs text to remind them what the value once was (since we don't have a visual indicator on the preview screen). You might be able to look at the logic for the
AltTextform, as it does use inputs as well.
There was a problem hiding this comment.
Form options work as expected, though the scanner seems to be having weird issues with how to flag incorrect lang attributes. For example, in a case like this in my canvas page, it only flags the first one:
<section lang="esd">Section lang remove language</section>
<section lang="frd">Section lang remove language</section>
<section lang="end">Section lang remove language</section>
even though all three are incorrect.
Not sure if there's anything we can do about that but the form looks good!
There's actually a TON of three-letter valid languages, and "frd" and "end" are two of them. There's a MASSIVE list in the new |
3 participants
Added language form