Display min/max limits with field description #15424

jhoward1994 · 2023-01-12T15:15:27Z

What does it do?

Adds the min and max constraints to field descriptions

How to test it?

Tests added for description util packages/core/helper-plugin/lib/src/hooks/useFieldHint/tests/useFieldHint.test.js

codecov · 2023-01-12T15:21:46Z

Codecov Report

Base: 61.27% // Head: 61.28% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (ea95c70) compared to base (a7df094).
Patch coverage: 57.14% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #15424   +/-   ##
=======================================
  Coverage   61.27%   61.28%           
=======================================
  Files        1355     1355           
  Lines       33499    33491    -8     
  Branches     6455     6449    -6     
=======================================
- Hits        20528    20526    -2     
+ Misses      11144    11142    -2     
+ Partials     1827     1823    -4

Flag	Coverage Δ
back	`50.62% <0.00%> (-0.01%)`	⬇️
front	`66.14% <100.00%> (+0.01%)`	⬆️
unit_back	`50.62% <0.00%> (-0.01%)`	⬇️
unit_front	`66.14% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...r/server/services/utils/configuration/metadatas.js	`9.80% <0.00%> (-0.20%)`	⬇️
...admin/src/content-manager/components/Hint/index.js	`81.81% <100.00%> (+0.56%)`	⬆️
...n/src/content-manager/components/InputUID/index.js	`72.56% <100.00%> (+0.15%)`	⬆️
...in/src/content-manager/components/Wysiwyg/index.js	`90.32% <100.00%> (ø)`
...ntent-manager/pages/EditView/Informations/index.js
...ager/pages/EditView/Informations/utils/getUnits.js
...ontent-manager/pages/EditView/Information/index.js	`100.00% <0.00%> (ø)`
...nager/pages/EditView/Information/utils/getUnits.js	`77.77% <0.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

joshuaellis · 2023-01-13T08:02:32Z

Your padding between the two lines & font weight on the character count don't look correct looking at the image you've shared. Would you be able to double check and revisit? At the moment it looks a bit lost.

packages/core/admin/admin/src/content-manager/components/Inputs/utils/fieldDescription.js

feat(content-manager): construct min/max description separately from main description

jhoward1994 · 2023-01-13T13:39:41Z

Your padding between the two lines & font weight on the character count don't look correct looking at the image you've shared. Would you be able to double check and revisit? At the moment it looks a bit lost.

Yeah I know what you mean. We had agreed as a first step to implement it like this (see CONTENT-863) but I agree it looks a bit strange.

WDYT @gu-stav ?

packages/core/admin/admin/src/content-manager/components/Inputs/index.js

joshuaellis · 2023-01-13T15:53:11Z

Yeah I know what you mean. We had agreed as a first step to implement it like this (see CONTENT-863) but I agree it looks a bit strange.

Ah i didn't see that this was agreed on. In that case leave it if @maevalienard is happy.

maevalienard · 2023-01-16T09:36:01Z

Well I'd rather see the min/max as semibold but it was easier as such :)

packages/core/helper-plugin/lib/src/index.js

packages/core/admin/admin/src/content-manager/components/Inputs/index.js

packages/core/helper-plugin/lib/src/hooks/useFieldHint/index.js

joshuaellis

Some feedback on your hook, i'd want to avoid disabling eslint rules for hooks where possible, i'd double check my comments before just committing them, i didn't double check what i wrote!

packages/core/admin/admin/src/content-manager/components/Inputs/utils/getMinMax.js

packages/core/helper-plugin/lib/src/components/GenericInput/utils/fieldUnits.js

packages/core/helper-plugin/lib/src/hooks/useFieldHint/index.js

fix(helper-plugin): useFieldHint dependencies array

joshuaellis

LGTM ⭐

gu-stav

It looks much better - good work! 🚀 I think there is only one tricky question left about this can be translated by the community, but maybe I just don't see the whole idea yet :)

A few notes/ questions from a functional test:

if I add the min (e.g. 10) validation to a field and leave the field empty while saving there is no validation error. This means the BE does not validate this constraint at the moment. Should we track this in a follow-up bug (max validation seems to work)?
there is no validation in case the "default value" entered in the CTB is longer than the allowed maximum, which might be a ux problem? Again: follow-up task?
for richtext fields currently these constraints are not displayed. I guess this is intentional?
integer / decimal fields display constraints, while biginteger fields do not. I guess you haven't touched that - should we track this in a follow-up issue?

packages/core/admin/admin/src/content-manager/components/Inputs/utils/getMinMax.js

packages/core/helper-plugin/lib/src/components/GenericInput/utils/fieldUnits.js

packages/core/helper-plugin/lib/src/hooks/useFieldHint/index.js

packages/core/helper-plugin/lib/src/hooks/useFieldHint/tests/useFieldHint.test.js

test(helper-plugin): getFieldUnits feat(I18n): add en.json translations for min/max descriptions feat(content-manager): support min/max descriptions in Wysiwyg and BIGINT inputs test(content-manager): getMinMax

jhoward1994 · 2023-01-19T16:11:10Z

It looks much better - good work! 🚀 I think there is only one tricky question left about this can be translated by the community, but maybe I just don't see the whole idea yet :)

A few notes/ questions from a functional test:

if I add the min (e.g. 10) validation to a field and leave the field empty while saving there is no validation error. This means the BE does not validate this constraint at the moment. Should we track this in a follow-up bug (max validation seems to work)?

there is no validation in case the "default value" entered in the CTB is longer than the allowed maximum, which might be a ux problem? Again: follow-up task?

for richtext fields currently these constraints are not displayed. I guess this is intentional?

integer / decimal fields display constraints, while biginteger fields do not. I guess you haven't touched that - should we track this in a follow-up issue?

Thanks for the review @gu-stav 🙏🏻, I've addressed your comments in the latest commits.

The only outstanding things are the validation issues you spotted. I agree these should be tracked in separate issues, I can make the JIRA tickets.

packages/core/helper-plugin/lib/src/components/GenericInput/utils/getFieldUnits.js

packages/core/helper-plugin/lib/src/hooks/useFieldHint/index.js

refactor(helper-plugin) fix(helper-plugin):combine translation strings

…iption

packages/core/admin/admin/src/translations/en.json

packages/core/helper-plugin/lib/src/hooks/useFieldHint/index.js

…iption

gu-stav

This looks good to me now! 🚀 Thank you!

packages/core/helper-plugin/lib/src/hooks/useFieldHint/index.js

joshuaellis

It's been a journey, but really great work 👍🏼

feat(content-manager): display min/max limits with field description

11ab76c

jhoward1994 added source: core:content-manager Source is core/content-manager package pr: feature This PR adds a new feature labels Jan 12, 2023

jhoward1994 self-assigned this Jan 12, 2023

jhoward1994 marked this pull request as ready for review January 12, 2023 16:30

jhoward1994 requested review from gu-stav, joshuaellis and ronronscelestes January 12, 2023 16:30

gu-stav suggested changes Jan 13, 2023

View reviewed changes

packages/core/admin/admin/src/content-manager/components/Inputs/utils/fieldDescription.js Outdated Show resolved Hide resolved

feat(content-manager): cover all field types

1eec599

feat(content-manager): construct min/max description separately from main description

gu-stav reviewed Jan 13, 2023

View reviewed changes

packages/core/admin/admin/src/content-manager/components/Inputs/index.js Outdated Show resolved Hide resolved

jhoward1994 added 2 commits January 16, 2023 16:43

feat(helper-plugin): handle hint generation

8c1fd4a

chore: code cleanup

89b0ad5

jhoward1994 requested a review from gu-stav January 17, 2023 11:17

gu-stav reviewed Jan 17, 2023

View reviewed changes

packages/core/helper-plugin/lib/src/index.js Outdated Show resolved Hide resolved

jhoward1994 added 2 commits January 17, 2023 13:55

fix(helper-lugin): pass field hint to UID component

c375c69

chore(helper-plugin)

3690ce3

gu-stav reviewed Jan 17, 2023

View reviewed changes

chore: feedback fixes

402617b

jhoward1994 requested review from gu-stav January 17, 2023 14:55

joshuaellis requested changes Jan 18, 2023

View reviewed changes

chore: docs

1b80447

fix(helper-plugin): useFieldHint dependencies array

jhoward1994 requested a review from joshuaellis January 18, 2023 15:10

joshuaellis previously approved these changes Jan 18, 2023

View reviewed changes

gu-stav reviewed Jan 19, 2023

View reviewed changes

fix(helper-plugin): make useFieldHint translatable

68984c2

test(helper-plugin): getFieldUnits feat(I18n): add en.json translations for min/max descriptions feat(content-manager): support min/max descriptions in Wysiwyg and BIGINT inputs test(content-manager): getMinMax

jhoward1994 dismissed joshuaellis’s stale review via 68984c2 January 19, 2023 15:55

jhoward1994 added 3 commits January 19, 2023 16:03

chore(helper-plugin)

9a47143

chore(content-manager)

7f7b4f2

chore(helper-plugin): useFieldHint variable naming

98fdd17

fix(helper-plugin): BigInt mistake

688baff

gu-stav reviewed Jan 20, 2023

View reviewed changes

packages/core/helper-plugin/lib/src/components/GenericInput/utils/getFieldUnits.js Outdated Show resolved Hide resolved

gu-stav reviewed Jan 20, 2023

View reviewed changes

packages/core/helper-plugin/lib/src/hooks/useFieldHint/index.js Outdated Show resolved Hide resolved

gu-stav mentioned this pull request Jan 20, 2023

Chore: Make field constraint error messages more consistent #15512

Merged

jhoward1994 added 2 commits January 20, 2023 12:41

fix(helper-plugin): make units translatable

c3fd52c

refactor(helper-plugin) fix(helper-plugin):combine translation strings

Merge remote-tracking branch 'origin/main' into feature/min-max-descr…

6150b67

…iption

jhoward1994 requested review from joshuaellis and gu-stav January 20, 2023 16:27

gu-stav reviewed Jan 23, 2023

View reviewed changes

packages/core/admin/admin/src/translations/en.json Outdated Show resolved Hide resolved

packages/core/helper-plugin/lib/src/hooks/useFieldHint/index.js Outdated Show resolved Hide resolved

jhoward1994 added 3 commits January 23, 2023 09:59

fix(helper-plugin): simplify translation string

2acdc52

Merge remote-tracking branch 'origin/main' into feature/min-max-descr…

057d325

…iption

chore(core)

a5bf7c1

jhoward1994 requested a review from gu-stav January 23, 2023 10:20

gu-stav previously approved these changes Jan 23, 2023

View reviewed changes

joshuaellis requested changes Jan 23, 2023

View reviewed changes

packages/core/helper-plugin/lib/src/hooks/useFieldHint/index.js Outdated Show resolved Hide resolved

refactor(helper-plugin): simplify useFieldHint

ea95c70

jhoward1994 dismissed gu-stav’s stale review via ea95c70 January 23, 2023 14:38

jhoward1994 requested a review from joshuaellis January 23, 2023 14:42

joshuaellis approved these changes Jan 23, 2023

View reviewed changes

jhoward1994 added this to the 4.6.0 milestone Jan 24, 2023

jhoward1994 merged commit 2153d4d into main Jan 24, 2023

jhoward1994 deleted the feature/min-max-description branch January 24, 2023 10:16

gu-stav mentioned this pull request Jan 30, 2023

[Audit logs] Add audit log filters #15504

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display min/max limits with field description #15424

Display min/max limits with field description #15424

jhoward1994 commented Jan 12, 2023 •

edited

codecov bot commented Jan 12, 2023 •

edited

joshuaellis commented Jan 13, 2023

jhoward1994 commented Jan 13, 2023

joshuaellis commented Jan 13, 2023

maevalienard commented Jan 16, 2023

joshuaellis left a comment

joshuaellis left a comment

gu-stav left a comment

jhoward1994 commented Jan 19, 2023

gu-stav left a comment

joshuaellis left a comment

Display min/max limits with field description #15424

Display min/max limits with field description #15424

Conversation

jhoward1994 commented Jan 12, 2023 • edited

What does it do?

How to test it?

codecov bot commented Jan 12, 2023 • edited

Codecov Report

joshuaellis commented Jan 13, 2023

jhoward1994 commented Jan 13, 2023

joshuaellis commented Jan 13, 2023

maevalienard commented Jan 16, 2023

joshuaellis left a comment

Choose a reason for hiding this comment

joshuaellis left a comment

Choose a reason for hiding this comment

gu-stav left a comment

Choose a reason for hiding this comment

jhoward1994 commented Jan 19, 2023

gu-stav left a comment

Choose a reason for hiding this comment

joshuaellis left a comment

Choose a reason for hiding this comment

jhoward1994 commented Jan 12, 2023 •

edited

codecov bot commented Jan 12, 2023 •

edited