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.

Sign up for GitHub

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

Jump to bottom

feat(combobox): add pending state #4462

Merged
merged 12 commits into from
Jun 5, 2024
Merged

feat(combobox): add pending state #4462

merged 12 commits into from
Jun 5, 2024

Conversation

Rocss
Copy link
Contributor

@Rocss Rocss commented May 16, 2024 •
edited

Description

Adds pending property for sp-combobox, enabling the pending state.

Spectrum CSS
Screenshot 2024-05-16 at 12 10 02

Related issue(s)

Motivation and context

A combo box can indicate that content is loading if system processes delay the display of the combo box content.

How has this been tested?

  • Test case 1
    1. Go here
    2. Observe documentation entries for quiet and pending states are now available
  • Test case 2
    1. Go here
    2. Combobox should receive focus when in pending state
    3. Your are able to type inside it, however, it does not open the suggestion list while it is pending
    4. Toggling the open control does not open the suggestion list
  • Test case 3
    1. Go here
    2. Inspect the combobox using Screen Reader (I used VoiceOver on Mac)
    3. It reads the "Pending" label together with the Combobox's visible label.
  • Test case 4
    1. Go here
    2. Toggle between LTR / RTL
    3. The progress circle is correctly positioned near the picker-button
  • Test case 5
    1. Go here
    2. Enable disabled control -> all looks good
  • Test case 6
    1. Go here
    2. Enable invalid control -> progress circle takes precedence over the alert icon
  • Test case 7
    1. Go here
    2. Enable quiet control -> all looks good

Screenshots (if appropriate)

Types of changes

  • 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 GitHub Actions
Copy link

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 GitHub Actions
Copy link

github-actions bot commented May 16, 2024 •
edited

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 221.482 kB 210.775 kB 🏆 210.778 kB
Scripts 53.548 kB 48.466 kB 48.372 kB 🏆
Stylesheet 35.004 kB 30.368 kB 🏆 30.504 kB
Document 5.981 kB 5.335 kB 5.269 kB 🏆
Font 126.949 kB 126.606 kB 🏆 126.633 kB

Request Count

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

Rocss
Rocss commented May 16, 2024
View reviewed changes
packages/combobox/src/Combobox.ts
@@ -402,6 +440,7 @@ export class Combobox extends Textfield {
: ''}"
?disabled=${this.disabled}
?focused=${this.focused}
?quiet=${this.quiet}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not really related to this PR but a very straight-forward fix for the quiet state

@Rocss Rocss linked an issue May 16, 2024 that may be closed by this pull request
[Feat]: Support pending state for Combobox #4382
Closed
1 task
@github-actions GitHub Actions
Copy link

github-actions bot commented May 16, 2024 •
edited

Tachometer results

Chrome

combobox permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 709 kB 35.63ms - 36.22ms - faster ✔
5% - 7%
1.91ms - 2.72ms
branch 697 kB 37.97ms - 38.52ms slower ❌
5% - 8%
1.91ms - 2.72ms		 -

light-dom-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 710 kB 394.66ms - 404.23ms - faster ✔
1% - 4%
3.75ms - 15.36ms
branch 698 kB 405.71ms - 412.29ms slower ❌
1% - 4%
3.75ms - 15.36ms		 -
Firefox

combobox permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 709 kB 60.15ms - 63.57ms - unsure 🔍
-3% - +3%
-1.92ms - +1.96ms
branch 697 kB 60.93ms - 62.75ms unsure 🔍
-3% - +3%
-1.96ms - +1.92ms		 -

light-dom-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 710 kB 725.04ms - 735.00ms - slower ❌
1% - 4%
5.79ms - 27.01ms
branch 698 kB 704.25ms - 722.99ms faster ✔
1% - 4%
5.79ms - 27.01ms		 -

@Rocss Rocss marked this pull request as ready for review May 16, 2024 10:33
@Rocss Rocss requested a review from a team May 16, 2024 10:33
Rajdeepc
Rajdeepc requested changes May 17, 2024
View reviewed changes
packages/combobox/src/Combobox.ts Outdated Show resolved Hide resolved
@Rajdeepc Rajdeepc requested a review from a team May 20, 2024 05:25
@Rajdeepc
Copy link
Contributor

Rajdeepc commented May 20, 2024 •
edited

This case shouldn't surface up!
In disabled state pending state should be undefined. Please confirm with design. Thanks

Screenshot 2024-05-20 at 11 41 42 AM

@TarunAdobe
Copy link
Contributor

Should a readonly combobox accept a pending state?
image

@Rajdeepc
Copy link
Contributor

Should a readonly combobox accept a pending state? image

https://spectrum.adobe.com/page/combo-box/ needs to be updated with pending state. I will convey it to the design team.

@Rocss
Copy link
Contributor Author

Rocss commented May 21, 2024 •
edited

Should a readonly combobox accept a pending state? image

@TarunAdobe mmmm in my opinion no, but I can not find any design docs on this :( Let's not allow pending if readonly, so that this is consistent with the sp-picker component for now. What do you think?

@Rajdeepc Agree with the disabled and pending not really working at the same time. Seems like in the sp-picker I allowed disabled and pending at the same time. Given that there are no design docs for now on this, should be enforce disabled to have a higher priority here?

@Rajdeepc
Copy link
Contributor

Should a readonly combobox accept a pending state? image

@TarunAdobe mmmm in my opinion no, but I can not find any design docs on this :( Let's not allow pending if readonly, so that this is consistent with the sp-picker component for now. What do you think?

@Rajdeepc Agree with the disabled and pending not really working at the same time. Seems like in the sp-picker I allowed disabled and pending at the same time. Given that there are no design docs for now on this, should be enforce disabled to have a higher priority here?

I have already send out a note to design to confirm this but yes if it is disabled, pending state should not be visible. It is also not accessible complaint. Let me know if you have any other questions!

@Rocss Rocss requested a review from Rajdeepc May 27, 2024 08:54
TarunAdobe
TarunAdobe approved these changes Jun 3, 2024
View reviewed changes
Copy link
Contributor

@TarunAdobe TarunAdobe left a comment

Choose a reason for hiding this comment

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

LGTM!! Thank you so much for your attention to detail. 🫰

@Rajdeepc Rajdeepc merged commit 2d0c388 into main Jun 5, 2024
58 checks passed
@Rajdeepc Rajdeepc deleted the rocss/pending-combobox branch June 5, 2024 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Westbrook Westbrook Westbrook left review comments

@Rajdeepc Rajdeepc Rajdeepc approved these changes

@TarunAdobe TarunAdobe TarunAdobe approved these changes

Assignees
No one assigned
Labels
Component: Combobox
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feat]: Support pending state for Combobox
4 participants
@Rocss @Rajdeepc @TarunAdobe @Westbrook