spectrum-web-components icon indicating copy to clipboard operation
spectrum-web-components copied to clipboard

Published 20 hours ago • verified publisher icon adobe

fix(progress-bar): removed duplicate label

Open nikkimk opened this issue 1 year ago • 16 comments
trafficstars

Description

Removes duplicate label from progress bar.

Related issue(s)

  • fixes #4225

How has this been tested?

  • [ ] Test case 1
    1. Go here
    2. Right click the first progress bar example and choose Inspect Element
    3. From the Element Inspector click on the progress bar HTML and select Edit HTML
    4. Replace the existing progress bar HTML the following HTML: <sp-progress-bar label="" size="m" indeterminate ></sp-progress-bar>.
    5. Click Enter.
    6. Verify that the new progress bar displayed has no label.
    7. Right click the new progress bar's HTML and choose Add Attribute.
    8. Type the attribute label="label1".
    9. Click Enter.
    10. Verify that the progress bar label displays as label1 not label1label1.

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

  • [x] I have signed the Adobe Open Source CLA.
  • [x] 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.
  • [x] I have read the CONTRIBUTING document.
  • [x] I have added tests to cover my changes. (Note: Testing won't allow an empty an empty label, so I can't test for this.)
  • [x] All new and existing tests passed.
  • [x] I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

nikkimk avatar May 23 '24 17:05 nikkimk

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[bot] avatar May 23 '24 17:05 github-actions[bot]

Lighthouse scores

Category Latest (report) Main (report) Branch (report)
Performance 0.99 0.99 0.99
Accessibility 1 1 1
Best Practices 1 1 1
SEO 1 0.92 0.92
PWA 1 1 1
What is this?

Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on main ("Main"). Higher scores are better, but note that the SEO scores on Netlify URLs are artifically constrained to 0.92.

Transfer Size

Category Latest Main Branch
Total 225.257 kB 212.067 kB 210.626 kB 🏆
Scripts 57.363 kB 49.723 kB 48.303 kB 🏆
Stylesheet 34.881 kB 30.341 kB 🏆 30.406 kB
Document 6.152 kB 5.338 kB 5.304 kB 🏆
Font 126.861 kB 126.665 kB 126.613 kB 🏆

Request Count

Category Latest Main Branch
Total 48 48 45 🏆
Scripts 40 40 37 🏆
Stylesheet 5 5 5
Document 1 1 1
Font 2 2 2

github-actions[bot] avatar May 23 '24 17:05 github-actions[bot]

Tachometer results

Chrome

progress-bar permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 408 kB 29.06ms - 29.75ms - slower ❌
2% - 5%
0.46ms - 1.40ms
branch 395 kB 28.16ms - 28.79ms faster ✔
2% - 5%
0.46ms - 1.40ms		 -
Firefox

progress-bar permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 408 kB 62.04ms - 69.08ms - slower ❌
0% - 13%
0.20ms - 7.92ms
branch 395 kB 59.92ms - 63.08ms faster ✔
1% - 12%
0.20ms - 7.92ms		 -

github-actions[bot] avatar May 23 '24 17:05 github-actions[bot]

@nikkimk Can you catch up to main and add some documentation on this?

Rajdeepc avatar May 28 '24 12:05 Rajdeepc

Can we add a relevant test for this

blunteshwar avatar May 28 '24 18:05 blunteshwar

I have added a couple of tests and updated the logic as well

blunteshwar avatar May 29 '24 08:05 blunteshwar

Think about how this will render <sp-progress-bar label="Loading" indeterminate>abcdefgh</sp-progress-bar>

@Rajdeepc What is the desired behavior for this use case?

nikkimk avatar May 29 '24 17:05 nikkimk

Think about how this will render <sp-progress-bar label="Loading" indeterminate>content</sp-progress-bar>

@Rajdeepc What is the desired behavior for this use case?

In this case <sp-progress-bar label="Loading" indeterminate>abcdefgh</sp-progress-bar> when both label is present and content are present then, both of them will exist inside slot sp-field-label which is logically not so correct. Either of them should be present in sp-filed-label. There shouldn't be a scenario when both label and slot content are present inside slot

blunteshwar avatar May 30 '24 06:05 blunteshwar

Think about how this will render <sp-progress-bar label="Loading" indeterminate>content</sp-progress-bar>

@Rajdeepc What is the desired behavior for this use case?

In this case <sp-progress-bar label="Loading" indeterminate>abcdefgh</sp-progress-bar> when both label is present and content are present then, both of them will exist inside slot sp-field-label which is logically not so correct. Either of them should be present in sp-filed-label. There shouldn't be a scenario when both label and slot content are present inside slot

So when both are present, is the attribute the only one that should exist?

nikkimk avatar May 30 '24 18:05 nikkimk

Think about how this will render <sp-progress-bar label="Loading" indeterminate>content</sp-progress-bar>

@Rajdeepc What is the desired behavior for this use case?

In this case <sp-progress-bar label="Loading" indeterminate>abcdefgh</sp-progress-bar> when both label is present and content are present then, both of them will exist inside slot sp-field-label which is logically not so correct. Either of them should be present in sp-filed-label. There shouldn't be a scenario when both label and slot content are present inside slot

So when both are present, is the attribute the only one that should exist?

When both are present only the content should exist which in the above example is 'abcdefgh'.

blunteshwar avatar May 31 '24 06:05 blunteshwar

When both are present only the content should exist which in the above example is 'abcdefgh'.

@Rajdeepc To make sure I'm understanding, is the following correct?

If label attribute is and default slot is then what appears visually should be and the aria-label should be
empty content content content
abcdefgh empty abcdefgh abcdefgh
abcdefgh content content abcdefgh
empty empty empty empty

nikkimk avatar May 31 '24 13:05 nikkimk

abcdefgh

In the third row when both label="abcdefgh" and content are present, then what appears visually should becontent

blunteshwar avatar Jun 03 '24 04:06 blunteshwar

@blunteshwar

abcdefgh

In the third row when both label="abcdefgh" and content are present, then what appears visually should becontent

What about the aria-label? Ideally sighted screenreader users should hear and see the same content?

nikkimk avatar Jun 03 '24 13:06 nikkimk

@blunteshwar

abcdefgh

In the third row when both label="abcdefgh" and content are present, then what appears visually should becontent

What about the aria-label? Ideally sighted screenreader users should hear and see the same content?

Indeed I think we have to make sure that when content is present then, aria-label =content.

blunteshwar avatar Jun 07 '24 08:06 blunteshwar

Making sure I capture that part of our discussion revolved around this previous issue: https://github.com/adobe/spectrum-web-components/issues/3146 and whether or not this is how progress bar should work.

nikkimk avatar Jun 19 '24 18:06 nikkimk

Making sure I capture that part of our discussion revolved around this previous issue: #3146 and whether or not this is how progress bar should work.

I think since this is a part of a longer discussion chain, how do we feel we fix the related bug of duplicate label and take this in a follow up work?

Rajdeepc avatar Jun 21 '24 13:06 Rajdeepc