Add URL for each candidate #133
Conversation
| // Choice contains a choice and an optional URL | ||
| type Choice struct { | ||
| Choice string | ||
| URL string |
There was a problem hiding this comment.
Could this have been an optional parameter?
No need to change at this state, just for next time.
| if (language === 'fr' && rank.ChoicesMap.ChoicesMap.has('fr')) | ||
| return choiceDisplay( | ||
| rank.ChoicesMap.ChoicesMap.get('fr')[choiceIndex], | ||
| rank.ChoicesMap.URLs[choiceIndex], | ||
| rankIndex | ||
| ); | ||
| else if (language === 'de' && rank.ChoicesMap.ChoicesMap.has('de')) | ||
| return choiceDisplay( | ||
| rank.ChoicesMap.ChoicesMap.get('de')[choiceIndex], | ||
| rank.ChoicesMap.URLs[choiceIndex], | ||
| rankIndex | ||
| ); | ||
| else if (rank.ChoicesMap.ChoicesMap.has('en')) |
There was a problem hiding this comment.
| if (language === 'fr' && rank.ChoicesMap.ChoicesMap.has('fr')) | |
| return choiceDisplay( | |
| rank.ChoicesMap.ChoicesMap.get('fr')[choiceIndex], | |
| rank.ChoicesMap.URLs[choiceIndex], | |
| rankIndex | |
| ); | |
| else if (language === 'de' && rank.ChoicesMap.ChoicesMap.has('de')) | |
| return choiceDisplay( | |
| rank.ChoicesMap.ChoicesMap.get('de')[choiceIndex], | |
| rank.ChoicesMap.URLs[choiceIndex], | |
| rankIndex | |
| ); | |
| else if (rank.ChoicesMap.ChoicesMap.has('en')) | |
| if (rank.ChoicesMap.ChoicesMap.has(language)) | |
| return choiceDisplay( | |
| rank.ChoicesMap.ChoicesMap.get(language)[choiceIndex], | |
| rank.ChoicesMap.URLs[choiceIndex], | |
| rankIndex | |
| ); | |
| else if (rank.ChoicesMap.ChoicesMap.has('en')) | |
| return choiceDisplay( | |
| rank.ChoicesMap.ChoicesMap.get('en')[choiceIndex], | |
| rank.ChoicesMap.URLs[choiceIndex], | |
| rankIndex | |
| ); |
which would get rid of the third if.
| const choiceDisplay = (isChecked: boolean, choice: string, choiceIndex: number) => { | ||
| const choiceDisplay = (isChecked: boolean, choice: string, url: string, choiceIndex: number) => { | ||
| const prettyChoice = url ? ( | ||
| <a href={url} style={{ color: 'blue', textDecoration: 'underline' }}> |
There was a problem hiding this comment.
Shouldn't the style be in double quotes? This creates an object and then does toString on it, doesn't it? But I haven't tested it.
| <a href={url} style={{ color: 'blue', textDecoration: 'underline' }}> | |
| <a href={url} style="color: 'blue', textDecoration: 'underline'"> |
There was a problem hiding this comment.
I followed another example from the existing code as I'm not very familiar with that 🤷♀️ when I'm working on the CSS I'll probably come back to it anyway
There was a problem hiding this comment.
I'm pretty sure it's correct what I write - but I was already surprised with other stuff in react. So probably Ahmed will know better.
| @@ -93,22 +100,25 @@ const Select: FC<SelectProps> = ({ select, answers, setAnswers, language }) => { | |||
| <div className="sm:pl-8 mt-2 pl-6"> | |||
| {Array.from(answers.SelectAnswers.get(select.ID).entries()) | |||
| .map(([choiceIndex, isChecked]) => { | |||
| if (language === 'fr' && select.ChoicesMap.has('fr')) | |||
| if (language === 'fr' && select.ChoicesMap.ChoicesMap.has('fr')) | |||
There was a problem hiding this comment.
| if (language === 'fr' && select.ChoicesMap.ChoicesMap.has('fr')) | |
| if (select.ChoicesMap.ChoicesMap.has(language)) |
Same as above, then you only have 2 ifs and it works even if there should be more languages.
| const columns = text.MaxLength > 50 ? 50 : text.MaxLength; | ||
| const prettyChoice = url ? ( | ||
| <a href={url} style={{ color: 'blue', textDecoration: 'underline' }}> |
There was a problem hiding this comment.
Same question as above wrt style.
| @@ -132,6 +133,9 @@ const AddQuestionModal: FC<AddQuestionModalProps> = ({ | |||
| return; | |||
| } | |||
| }; | |||
| const choices = ChoicesMap.ChoicesMap.has(language) | |||
There was a problem hiding this comment.
This might come out as undefined. But I guess we suppose that en is always present. Is there a test for that in the editing?
| )[index]; | ||
| const url = choicesMap.URLs[index]; | ||
| return url ? ( | ||
| <a href={url} style={{ color: 'blue', textDecoration: 'underline' }}> |
| ChoicesMap.get('en').length + 1, | ||
| ChoicesMap.get('fr').length + 1, | ||
| ChoicesMap.get('de').length + 1 | ||
| ChoicesMap.ChoicesMap.get('en').length + 1, |
There was a problem hiding this comment.
I propose to replace it with this (thanks to Claude):
Math.max(...ChoicesMap.ChoicesMap.values()) + 1
Which will even work if some of the languages are not present.
| setState({ | ||
| ...state, | ||
| ChoicesMap: ChoicesMap, | ||
| MaxN: ChoicesMap.get('en').length, | ||
| MinN: ChoicesMap.get('en').length, | ||
| MaxN: ChoicesMap.ChoicesMap.get('en').length, |
There was a problem hiding this comment.
Too lazy to follow all the code: why do you default to en here instead of using Math.max?
There was a problem hiding this comment.
I just adapted the existing code (also applies to some of your other comments), didn't feel like rewriting too much at this stage as it would imply even more testing... I guess when I add the automated tests for the form creation I'll come back to this code anyway
This PR adds an URL field to the choices, and displays it in the form of a clickable label. It also fixes the internalisation for Rank/Text displays. Closes #37 .