Skip to content
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

feat: dynamic unit on input number #803

Open
wants to merge 10 commits into
base: 1.7
Choose a base branch
from
Open

feat: dynamic unit on input number #803

wants to merge 10 commits into from

Conversation

QRuhier
Copy link
Contributor

@QRuhier QRuhier commented Sep 11, 2024

@QRuhier QRuhier changed the title feat: dynamic unit feat: dynamic unit on input number Sep 11, 2024
Copy link

sonarcloud bot commented Nov 29, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
30.2% Coverage on New Code (required ≥ 50%)
9.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@@ -521,10 +521,18 @@ const dictionary = {
en: 'Please respect the date format',
fr: 'Merci de respecter le format de la date',
},
dynamicUnit: {
en: 'Custom unit of measure',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Customized unit of measurement

unit: {
en: 'Unit of measure',
fr: 'Unité de mesure',
},
dynamicUnitFormula: {
en: 'Formula of the unit of measure',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit of measurement formula

@@ -135,7 +135,7 @@ describe('response tranformations', () => {
expect(result.Datatype.Maximum).toEqual(maximum);
});
test('when Decimals is defined', () => {
const typeName = 'DATE';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it not 'DATE' anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it actually does not change anything because we manage it in the same way whatever the type

// Convert string "true"/"false" to boolean true/false when storing in Redux form
parse={value => value === 'true'}
// Convert boolean true/false to string "true"/"false" when displaying the form
format={value => (value === true ? 'true' : 'false')}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To simplify we could have defined this function as:

  value => `${value}`

But to each their own

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll change the comment, actually value can be undefined and i want to handle it as a false value

// Convert boolean true/false to string "true"/"false" when displaying the form
format={value => (value === true ? 'true' : 'false')}
>
<GenericOption key="1" value="true">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the key is meant as an React key to identify an item in a list, it is better practice to use some sort of id instead of an index (eg "dictionary-true" instead of "1") since the key id should never change for each item.

See: https://react.dev/learn/rendering-lists#keeping-list-items-in-order-with-key

Here it doesn't seem to be useful since it's not an array though so we should remove this property since key does not seem to be used by the GenericOption component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants