fix(number-field): update button label to use number-field-labels as part of the text #3474

TarunAdobe · 2023-07-19T05:52:01Z

Updated the Number field increase/decrease buttons to use the number fields label as part of their label text

Description

Previously the number field increase/decrease buttons were using hardcoded labels as Increment/Decrement but now with this update buttons label text would use the number-field-label text as a part of it to give more context and clarity.

So for

<sp-field-label for="units">Package width</sp-field-label>
<sp-number-field
    id="units"
    style="width: 200px"
    value="4"
    format-options='{
        "style": "unit",
        "unit": "inch",
        "unitDisplay": "long"
    }'
></sp-number-field>

The buttons would have their labels as "Increase Package width" and "Decrease Package Width".

Related issue(s)

fixes [Bug]: sp-number-field Increment/Decrement button labels #3273

Motivation and context

How has this been tested?

Test case 1
1. Go here
2. Do this
Test case 2
1. Go here
2. Do this

Screenshots (if appropriate)

Types of changes

[x ] Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
If my change required a change to the documentation, I have updated the documentation in this pull request.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

github-actions · 2023-07-19T05:55:24Z

Branch Preview

Visual regression test results

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

github-actions · 2023-07-19T06:02:36Z

Tachometer results

Chrome

number-field permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	625 kB	228.52ms - 233.67ms	-	faster ✔ 0% - 3% 0.38ms - 7.49ms
branch	626 kB	232.58ms - 237.48ms	slower ❌ 0% - 3% 0.38ms - 7.49ms	-

search permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	469 kB	115.90ms - 117.44ms	-	unsure 🔍 -5% - +2% -5.69ms - +2.50ms
branch	468 kB	114.24ms - 122.28ms	unsure 🔍 -2% - +5% -2.50ms - +5.69ms	-

slider permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	452 kB	162.57ms - 166.80ms	-	unsure 🔍 -2% - +2% -2.76ms - +2.74ms
branch	453 kB	162.94ms - 166.44ms	unsure 🔍 -2% - +2% -2.74ms - +2.76ms	-

textfield permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	416 kB	68.97ms - 70.42ms	-	unsure 🔍 -2% - +1% -1.42ms - +0.64ms
branch	416 kB	69.35ms - 70.81ms	unsure 🔍 -1% - +2% -0.64ms - +1.42ms	-

Firefox

number-field permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	625 kB	423.64ms - 448.76ms	-	faster ✔ 4% - 11% 20.01ms - 51.51ms
branch	626 kB	462.46ms - 481.46ms	slower ❌ 4% - 12% 20.01ms - 51.51ms	-

search permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	469 kB	203.63ms - 228.33ms	-	unsure 🔍 -11% - +2% -25.88ms - +5.48ms
branch	468 kB	216.52ms - 235.84ms	unsure 🔍 -3% - +12% -5.48ms - +25.88ms	-

slider permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	452 kB	341.34ms - 362.46ms	-	unsure 🔍 -4% - +5% -12.91ms - +16.87ms
branch	453 kB	339.43ms - 360.41ms	unsure 🔍 -5% - +4% -16.87ms - +12.91ms	-

textfield permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	416 kB	148.02ms - 166.18ms	-	unsure 🔍 -3% - +15% -4.20ms - +21.48ms
branch	416 kB	139.38ms - 157.54ms	unsure 🔍 -13% - +2% -21.48ms - +4.20ms	-

Rajdeepc

LGTM!

Westbrook · 2023-07-19T11:45:58Z

Please review the failing tests in this branch.

TarunAdobe · 2023-07-19T13:01:15Z

@Westbrook Here's some explanation for these changes... I'd love to get your opinion on this.
So in the FieldLabel we have this function applyTargetLabel which we use to apply target label to other components. Now let's say we talk about number-field component. In that we have the focusable set as the input so the label of input is applied automatically from the applyTargetLabel function. We also wanted to have labels on the buttons but the buttons are not focusable (the input is). Thus is we use a separate helper function that is provided namely applyFocusElementLabel then according to the older conditions we will never apply the label to the focusable element. Thus I modified the if/else such that it would always run the applyLabel function if it is provided by the component and on top of that it will do other house keeping tasks like applying label to the focus element. I wanna understand if this is the correct way to do this? Thnx.
(Refer to latest changes for better understanding)

packages/field-label/src/FieldLabel.ts

TarunAdobe · 2023-07-20T11:05:46Z

updated the logic such that textField takes care of setting up the labels on the input properly

Westbrook

This change is looking good, just some small clean up remains.

Together with that, please take a look at this test and include a similar one here to support preventing regressions in this area going forward.

packages/number-field/src/NumberField.ts

packages/field-label/src/FieldLabel.ts

TarunAdobe · 2023-07-21T09:25:12Z

updated the code to add a test and fixed the code styling issues @Westbrook

Westbrook

This looks great. Thanks for adding the test.

…ble elements

…uselementlabel

TarunAdobe mentioned this pull request Jul 19, 2023

[Bug]: sp-number-field Increment/Decrement button labels #3273

Closed

1 task

TarunAdobe self-assigned this Jul 19, 2023

TarunAdobe requested review from Westbrook, Rajdeepc and najikahalsema July 19, 2023 05:53

Rajdeepc previously approved these changes Jul 19, 2023

View reviewed changes

TarunAdobe dismissed Rajdeepc’s stale review via a310b26 July 19, 2023 12:34

Westbrook reviewed Jul 19, 2023

View reviewed changes

packages/field-label/src/FieldLabel.ts Outdated Show resolved Hide resolved

Westbrook reviewed Jul 20, 2023

View reviewed changes

packages/number-field/src/NumberField.ts Outdated Show resolved Hide resolved

packages/field-label/src/FieldLabel.ts Outdated Show resolved Hide resolved

Westbrook approved these changes Jul 21, 2023

View reviewed changes

Tarun Tomar added 5 commits July 21, 2023 07:48

fix(number-field): update button label text

65ab98c

fix(field-label): updated conditions to apply target labels on focusa…

986c109

…ble elements

fix(textfield): update textfield to use appliedlabel incase of no foc…

4b31501

…uselementlabel

fix(number-field): added test to check labels on buttons

9fc5f3e

fix(number-field): fixed code styling issues

118da9c

Westbrook force-pushed the issue/number-field-labels branch from 9c139a9 to 118da9c Compare July 21, 2023 11:49

Westbrook merged commit b92daf2 into main Jul 21, 2023
6 of 46 checks passed

Westbrook deleted the issue/number-field-labels branch July 21, 2023 11:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(number-field): update button label to use number-field-labels as part of the text #3474

fix(number-field): update button label to use number-field-labels as part of the text #3474

TarunAdobe commented Jul 19, 2023 •

edited by Westbrook

github-actions bot commented Jul 19, 2023

github-actions bot commented Jul 19, 2023 •

edited

number-field permalink

search permalink

slider permalink

textfield permalink

number-field permalink

search permalink

slider permalink

textfield permalink

Rajdeepc left a comment

Westbrook commented Jul 19, 2023

TarunAdobe commented Jul 19, 2023

TarunAdobe commented Jul 20, 2023

Westbrook left a comment

TarunAdobe commented Jul 21, 2023

Westbrook left a comment

fix(number-field): update button label to use number-field-labels as part of the text #3474

fix(number-field): update button label to use number-field-labels as part of the text #3474

Conversation

TarunAdobe commented Jul 19, 2023 • edited by Westbrook

Description

Related issue(s)

Motivation and context

How has this been tested?

Screenshots (if appropriate)

Types of changes

Checklist

Best practices

github-actions bot commented Jul 19, 2023

Branch Preview

github-actions bot commented Jul 19, 2023 • edited

Tachometer results

number-field permalink

search permalink

slider permalink

textfield permalink

number-field permalink

search permalink

slider permalink

textfield permalink

Rajdeepc left a comment

Choose a reason for hiding this comment

Westbrook commented Jul 19, 2023

TarunAdobe commented Jul 19, 2023

TarunAdobe commented Jul 20, 2023

Westbrook left a comment

Choose a reason for hiding this comment

TarunAdobe commented Jul 21, 2023

Westbrook left a comment

Choose a reason for hiding this comment

TarunAdobe commented Jul 19, 2023 •

edited by Westbrook

github-actions bot commented Jul 19, 2023 •

edited