-
Notifications
You must be signed in to change notification settings - Fork 9
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
InputChoiceSorting: Rework logic to work horizontally. #333
base: main
Are you sure you want to change the base?
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.
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! 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run yarn format
on the code, this looks misaligned
@@ -263,6 +271,47 @@ export class InputCheckbox<CustomSurvey, CustomHousehold, CustomHome, CustomPers | |||
|
|||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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.
Yes
); | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -4,87 +4,60 @@ | |||
* This file is licensed under the MIT License. | |||
* License text available at https://opensource.org/licenses/MIT | |||
*/ | |||
import { DESTRUCTION } from 'dns'; |
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 is probably unused?
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.
Please remove
)} | ||
<input | ||
type="text" | ||
tabIndex={-1} |
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.
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}
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.
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.
@@ -4,87 +4,60 @@ | |||
* This file is licensed under the MIT License. | |||
* License text available at https://opensource.org/licenses/MIT | |||
*/ | |||
import { DESTRUCTION } from 'dns'; |
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.
Please remove
{columnWidgets} | ||
</div> | ||
); | ||
if (this.props.widgetConfig.alignment === undefined || this.props.widgetConfig.alignment === 'vertical') { |
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.
Here, in this if, the undefined alignment is considered as vertical
@@ -273,11 +281,15 @@ export class InputCheckbox<CustomSurvey, CustomHousehold, CustomHome, CustomPers | |||
) | |||
: null; | |||
|
|||
const shouldDisplayAsRows = | |||
this.props.widgetConfig.alignment === undefined || this.props.widgetConfig.alignment === 'auto'; |
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.
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 |
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: