fix(ui-library): ensure property naming number field (831) #940

JpunktWpunkt · 2024-02-21T08:51:55Z

related to #831 and #834

thrbnhrtmnn

hasUnit is not working as intended in Storybook. Instead of a boolean there should be three options, that can be selected via a radio: "none", "prefix", "suffix" with "none" being the default.

If hasUnit is either "prefix" or "suffix", the "unit" prop should be shown in the props panel. If hasUnit="none", the "unit" prop should not be shown.

Also in Storybook the FormCaptionGroup example story under "Dependencies" is missing a hint message in both instances and also a hint message icon in the second instance.

thrbnhrtmnn

Hey @JpunktWpunkt , the changes already look really good. I would just change a few minor things, before I will approve:

unitPosition should per default be "prefix" and we should not offer the "none" option
hasUnit please update the Description in the props panel to "Choose if component has a unit.". We should be a bit more consistent with similar props like hasLabel
Please align the order of the props in the props panel to the Excel (I think only unitPosition and the section "Accessibility" and "Technical Attributes" need to be adjusted)

thrbnhrtmnn

Storybook looks perfect now!

By the way, did you also solve #834 with the implementation? If so, please let me know so I can close the other issue :-)

JpunktWpunkt · 2024-03-22T08:39:48Z

Storybook looks perfect now!

By the way, did you also solve #834 with the implementation? If so, please let me know so I can close the other issue :-)

Yes, I did :) #834 with the last commit when I checked the order I saw that name was missing :)

angsherpa456 · 2024-03-22T09:12:30Z

packages/ui-library/src/components/input-field-number/index.ts

@@ -335,7 +337,9 @@ export class BlrInputFieldNumber extends LitElement {
                @select=${this.handleSelect}
                placeholder=${this.placeholder || ''}
              />
-              ${hasUnit ? html` <div class="${unitClasses}">${this.unit}</div> ` : nothing}
+              ${this.unitPosition === 'prefix' || this.unitPosition === 'suffix'
+                ? html` <div class="${unitClasses}">${this.unit}</div> `


Please remove the extra space on both side of html

angsherpa456 · 2024-03-22T09:13:13Z

packages/ui-library/src/components/input-field-number/index.ts

@@ -257,40 +259,40 @@ export class BlrInputFieldNumber extends LitElement {
        [`${this.sizeVariant}`]: this.sizeVariant,
        [this.stepperVariant || 'split']: this.stepperVariant || 'split',
        'error-input': this.hasError || false,
-        'prepend': hasUnit && !!this.prependUnit,
+        // 'prepend': this.unitPosition === 'prefix' || this.unitPosition === 'suffix',


Can this be removed?

angsherpa456 · 2024-03-22T09:31:58Z

packages/ui-library/src/components/input-field-number/index.stories.ts

    },
+


Please remove this additional line.

angsherpa456

LGTM

The merge-base changed after approval.

angsherpa456

LGTM

RubirajAccenture previously approved these changes Feb 21, 2024

View reviewed changes

JpunktWpunkt linked an issue Feb 21, 2024 that may be closed by this pull request

Number Input - Ensure Consistency between Storybook Documentation and Codebase Property Names #831

Closed

6 tasks

angsherpa456 previously approved these changes Feb 21, 2024

View reviewed changes

JpunktWpunkt dismissed stale reviews from angsherpa456 and RubirajAccenture via 6c22486 February 21, 2024 10:10

JpunktWpunkt requested review from RubirajAccenture and angsherpa456 February 21, 2024 12:06

angsherpa456 previously approved these changes Feb 21, 2024

View reviewed changes

JpunktWpunkt dismissed angsherpa456’s stale review via 03427aa February 21, 2024 12:13

angsherpa456 previously approved these changes Feb 21, 2024

View reviewed changes

JpunktWpunkt dismissed angsherpa456’s stale review via b242882 February 21, 2024 13:31

JpunktWpunkt requested a review from angsherpa456 February 21, 2024 13:35

angsherpa456 previously approved these changes Feb 21, 2024

View reviewed changes

davidken91 previously approved these changes Feb 21, 2024

View reviewed changes

thrbnhrtmnn requested changes Feb 26, 2024

View reviewed changes

JpunktWpunkt dismissed stale reviews from davidken91 and angsherpa456 via 583516a March 6, 2024 12:38

JpunktWpunkt marked this pull request as draft March 8, 2024 09:53

ChristianHoffmannS2 changed the title ~~Fix/831 ensure property naming number field~~ fix(ui-library): ensure property naming number field (831) Mar 8, 2024

JpunktWpunkt marked this pull request as ready for review March 21, 2024 13:19

thrbnhrtmnn requested changes Mar 21, 2024

View reviewed changes

JpunktWpunkt requested review from thrbnhrtmnn, davidken91 and angsherpa456 March 21, 2024 19:51

thrbnhrtmnn previously approved these changes Mar 22, 2024

View reviewed changes

JpunktWpunkt linked an issue Mar 22, 2024 that may be closed by this pull request

Number Input - Add Name Property #834

Closed

2 tasks

angsherpa456 reviewed Mar 22, 2024

View reviewed changes

JpunktWpunkt requested a review from angsherpa456 March 22, 2024 09:25

angsherpa456 reviewed Mar 22, 2024

View reviewed changes

JpunktWpunkt dismissed thrbnhrtmnn’s stale review via 5965e97 March 22, 2024 09:42

JpunktWpunkt requested review from thrbnhrtmnn and angsherpa456 March 22, 2024 09:45

angsherpa456 previously approved these changes Mar 22, 2024

View reviewed changes

JpunktWpunkt dismissed angsherpa456’s stale review via 1c4a456 March 22, 2024 13:32

JpunktWpunkt added the 🦹 needs:reviewers (code) The issue or pull request needs additional code reviewers label Mar 22, 2024

faselbaum removed the request for review from davidken91 March 22, 2024 14:23

thrbnhrtmnn previously approved these changes Mar 22, 2024

View reviewed changes

ChristianHoffmannS2 force-pushed the develop branch 2 times, most recently from 358f467 to 99c0c93 Compare March 26, 2024 13:22

thrbnhrtmnn requested review from thrbnhrtmnn and removed request for thrbnhrtmnn March 27, 2024 09:05

thrbnhrtmnn removed the 🦹 needs:reviewers (code) The issue or pull request needs additional code reviewers label Mar 27, 2024

thrbnhrtmnn self-requested a review March 27, 2024 09:07

JpunktWpunkt changed the base branch from develop to develop_old March 27, 2024 10:16

faselbaum force-pushed the fix/831-ensure-property-naming-number-field branch from 1c4a456 to a4a5c19 Compare March 27, 2024 15:02

faselbaum changed the base branch from develop_old to develop March 27, 2024 15:02

fix(ui-library): harmonize property naming

e5d178e

faselbaum force-pushed the fix/831-ensure-property-naming-number-field branch from a4a5c19 to e5d178e Compare March 27, 2024 15:38

thrbnhrtmnn removed the request for review from RubirajAccenture March 27, 2024 15:38

faselbaum self-requested a review March 27, 2024 15:41

faselbaum approved these changes Mar 27, 2024

View reviewed changes

faselbaum requested a review from angsherpa456 March 27, 2024 15:42

angsherpa456 approved these changes Mar 27, 2024

View reviewed changes

JpunktWpunkt merged commit d4ab9b7 into develop Mar 27, 2024
4 of 5 checks passed

JpunktWpunkt deleted the fix/831-ensure-property-naming-number-field branch March 27, 2024 16:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ui-library): ensure property naming number field (831) #940

fix(ui-library): ensure property naming number field (831) #940

JpunktWpunkt commented Feb 21, 2024 •

edited

Loading

thrbnhrtmnn left a comment

thrbnhrtmnn left a comment

thrbnhrtmnn left a comment

JpunktWpunkt commented Mar 22, 2024 •

edited

Loading

angsherpa456 Mar 22, 2024

angsherpa456 Mar 22, 2024

angsherpa456 Mar 22, 2024

angsherpa456 left a comment

angsherpa456 left a comment

fix(ui-library): ensure property naming number field (831) #940

fix(ui-library): ensure property naming number field (831) #940

Conversation

JpunktWpunkt commented Feb 21, 2024 • edited Loading

thrbnhrtmnn left a comment

Choose a reason for hiding this comment

thrbnhrtmnn left a comment

Choose a reason for hiding this comment

thrbnhrtmnn left a comment

Choose a reason for hiding this comment

JpunktWpunkt commented Mar 22, 2024 • edited Loading

angsherpa456 Mar 22, 2024

Choose a reason for hiding this comment

angsherpa456 Mar 22, 2024

Choose a reason for hiding this comment

angsherpa456 Mar 22, 2024

Choose a reason for hiding this comment

angsherpa456 left a comment

Choose a reason for hiding this comment

angsherpa456 left a comment

Choose a reason for hiding this comment

JpunktWpunkt commented Feb 21, 2024 •

edited

Loading

JpunktWpunkt commented Mar 22, 2024 •

edited

Loading