InputChoiceSorting: Rework logic to work horizontally.#333
Conversation
Rework logic to use better css classes and work with Flex to be able to set custom lengths freely horizontally. The horizontal limitation of the older logic is no more and should be more flexible.
tahini
left a comment
tahini
left a comment
There was a problem hiding this comment.
Thanks! seems to work, I,m no css expert, maybe @kaligrafy or @davidmurray can take a look. A few comments on the code though, and please run yarn format before submitting.
| ) | ||
| : null; | ||
|
|
||
| return ( |
There was a problem hiding this comment.
Run yarn format on the code, this looks misaligned
| // separate by columns if needed: | ||
| const columnedChoiceInputs = this.getColumnedChoices(choiceInputs); | ||
| if (this.props.widgetConfig.alignment === undefined || this.props.widgetConfig.alignment === 'vertical') { | ||
| const strCustomLabel = this.props.widgetConfig.customLabel |
There was a problem hiding this comment.
This statement is identical to the one after the if below (I think). If so, take it out of the if, above it, so you don't have to copy-paste it.
| <span>{strCustomLabel}</span> | ||
| </label> | ||
| )} | ||
| <input |
There was a problem hiding this comment.
This whole block is very similar to the one of the else, below this if. What is the difference? If it is only the class, you can do your if only in this one place, to avoid too much code duplication. If there are many differences, it is worth doing a separate block, with a comment explaining the difference. Otherwise, the code is harder to read as it is harder to understand the diff between the 2 blocks.
| default: | ||
| return verticalByColumns(arr, 2); | ||
| } | ||
| return splitSort(arr, 2); |
There was a problem hiding this comment.
iiuc, what this PR changes also is that the alignment is now the responsibility of the widget/css instead of this group sorting, as before?
| ); | ||
| if (this.props.widgetConfig.alignment === undefined || this.props.widgetConfig.alignment === 'vertical') { | ||
| columnedChoiceInputs.push( | ||
| <div className="survey-question__input-radio-group-column" key={i}> |
There was a problem hiding this comment.
Here too, instead of the if-then-else, you can use
className=`survey-question__input-radio-group-{this.props.widgetConfig.alignment === undefined || this.props.widgetConfig.alignment === 'vertical' ? 'column' : 'row' }`
to avoid code duplication.
| * This file is licensed under the MIT License. | ||
| * License text available at https://opensource.org/licenses/MIT | ||
| */ | ||
| import { DESTRUCTION } from 'dns'; |
| )} | ||
| <input | ||
| type="text" | ||
| tabIndex={-1} |
There was a problem hiding this comment.
peut-être que ce code va faire en sorte que l'élément de sera pas sélectionnable avec le clavier. Est-ce le cas?
Si oui il faudrait retirer tabIndex={-1}
tahini
left a comment
tahini
left a comment
There was a problem hiding this comment.
You may want to add a few unit tests for either InputRadio or InputCheckbox to make sure the right css is picked: in a describe block, add 4 tests with each of the alignments types, mixed with custom alignement length and columns/rows, etc. It should be fairly easy to add snapshot tests and make sure the result is as expected.
| * This file is licensed under the MIT License. | ||
| * License text available at https://opensource.org/licenses/MIT | ||
| */ | ||
| import { DESTRUCTION } from 'dns'; |
| {columnWidgets} | ||
| </div> | ||
| ); | ||
| if (this.props.widgetConfig.alignment === undefined || this.props.widgetConfig.alignment === 'vertical') { |
There was a problem hiding this comment.
Here, in this if, the undefined alignment is considered as vertical
| : null; | ||
|
|
||
| const shouldDisplayAsRows = | ||
| this.props.widgetConfig.alignment === undefined || this.props.widgetConfig.alignment === 'auto'; |
There was a problem hiding this comment.
In this one, the undefined should be displayed as rows.
1- Is it normal or a mistake? If normal, please add a comment explaining why in the code
2- The alignment type says alignment?: 'vertical' | 'horizontal' | 'auto'. I think you should add a || for the horizontal case here.
|
@tahini , should we incorporate this or forget about it? |
|
Do we have a use case for this? I don't see any. |
|
We had a use case in od_mtl, see the issue linked at the beginning of the PR. I think we can keep it and re-work |
5 participants
Rework logic to use better css classes and work with Flex to be able to set custom lengths freely horizontally. The horizontal limitation of the older logic is no more and should be more flexible.
Ex:
