feat: deprecate sp-menu in PickerBase derived classes #1237

adixon-adobe · 2021-03-02T16:08:27Z

Description

This is an update to #1198 with the functional approach. It should also address some of the test failures there.

I'll leave as a draft until I update all the related docs/tests.

Related Issue

#1235

Motivation and Context

Simpler APIs are generally better.

How Has This Been Tested?

Just some basic doc page testing so far.

Screenshots (if appropriate):

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
[] I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
[] I have added tests to cover my changes.
[] All new and existing tests passed.

packages/picker/src/Picker.ts

adixon-adobe · 2021-03-02T17:59:41Z

@Westbrook there's still a few SplitButton failures, and I now see that the markup is a little different for it.

I'm wondering if you've thought of any alternatives to the current usage that requires the popover to be created by the client. I'd like to try to simplify that usage as well as part of this change, but there's definitely a conflict between the Button & Picker patterns here.

Westbrook

While you're here can you confirm that the slot documentation for each of these element is correct based on this change (not to say it was 100% correct before). For instance, this was wrong before and is more wrong now: https://github.com/adobe/spectrum-web-components/blob/main/packages/split-button/src/SplitButton.ts#L44 Action Menu looks like it has the same twice out of date listing while Picker seems like it just needs to be specifically updated for this change.

packages/picker/src/Picker.ts

Westbrook · 2021-03-03T04:33:00Z

packages/picker/src/Picker.ts

        super.firstUpdated(changedProperties);

-        this.optionsMenu = this.querySelector('sp-menu') as Menu;
+        this.optionsMenu = this.shadowRoot.querySelector('sp-menu') as Menu;


With this in the shadow root we should like refactor to:

@query('sp-menu') private optionsMenu!: Menu;

There's even a caching mechanism in the query decorator we can activate if we'd like.

oh hey, we can. Nice!

oh hey, we can't quite (this still gets reparented by the popover, so that refactor leads to null results when the menu is open)

Double-check my changes here. Is it okay to have the optionsMenu! if it gets initialized in firstUpdated?

In theory @query('sp-menu', true) would cache this, but if the timing still doesn't work out right, no problem.

We "know" the options menu will be set everywhere it's used, so we can still use the ! in my oppinion.

packages/picker/test/picker.test.ts

packages/shared/src/reparent-children.ts

packages/split-button/README.md

packages/picker/src/Picker.ts

Westbrook · 2021-03-03T13:10:13Z

Also, the SplitButton test failures all originate in how that component manages it's items a little different: https://github.com/adobe/spectrum-web-components/blob/main/packages/split-button/src/SplitButton.ts#L180

Westbrook · 2021-03-03T23:15:47Z

packages/picker/src/Picker.ts

        super.firstUpdated(changedProperties);

-        this.optionsMenu = this.querySelector('sp-menu') as Menu;
+        this.optionsMenu = this.shadowRoot.querySelector('sp-menu') as Menu;


In theory @query('sp-menu', true) would cache this, but if the timing still doesn't work out right, no problem.

We "know" the options menu will be set everywhere it's used, so we can still use the ! in my oppinion.

packages/split-button/src/SplitButton.ts

Westbrook · 2021-03-03T23:23:18Z

packages/picker/src/Picker.ts

-        if (this.optionsMenu.menuItems.length) {
-            this.manageSelection();
+        if (this.open && this.optionsMenu) {
+            await this.optionsMenu.updateComplete;


For reference in this example, with the sp-menu removed, you'd get this.menuItems === [] and this.optionsMenu.menuItems === [MenuItem, MenuItem, MenuItem, MenuItem]

adixon-adobe · 2021-03-04T03:26:48Z

packages/picker/src/Picker.ts

+        if (!this.open) {
+            this.menuItems = [
+                ...this.querySelectorAll(`sp-menu-item`),
+            ] as MenuItem[];


My thinking here is that it should be okay to change the menu items, but that it works best when the menu is closed.

adixon-adobe · 2021-03-04T04:19:54Z

It'd be good to update the picker tests so the slotted content tests actually open/select something (and we validate that the slotted content wasn't reparented).

It'd also be good to add a test where we add or remove a menu item and ensure that things still work (and the newly added item is selectable).

I might need some help with those though, and if this otherwise looks good it might make sense to add an issue and merge.

adixon-adobe · 2021-03-04T04:50:36Z

I'm seeing some errors in the storybook pages now that I'm digging deeper, so hold on any merge here.

adixon-adobe · 2021-03-04T21:03:12Z

@Westbrook I think I've addressed all the feedback you had (except @query('sp-menu', true) which doesn't work.

I can add a separate issue for improving some of the tests here. I know the visual diffs caught some things the tests could have caught earlier (but that also means there's at least a little coverage of some scenarios I noticed the tests were missing).

packages/picker/src/Picker.ts

There's still work to update all the tests/docs, but if this is working right most tests should pass.

Also properly import sp-menu for usage in PickerBase

I'd do more on the manageSelection changes here, but I think we can remove a lot of this by using new options from #1189

@query

The @query wasn't working here -- ended up with null deference errors. Also need to initialize menuItems in firstUpdated in case the menu state is already open. Also revert an aria-role change that shouldn't have been committed. Bad late-night flawgic.

Westbrook

LGTM.

Would love to see reparentChildren leveraged in ActiveOverlay as a stand along PR before we get to the bigger menu changes you'll be looking at next. The reusability of that API is a really great addition to the library! Thanks for the deep work here.

adixon-adobe requested a review from Westbrook March 2, 2021 16:08

adixon-adobe commented Mar 2, 2021

View reviewed changes

packages/picker/src/Picker.ts Outdated Show resolved Hide resolved

Westbrook suggested changes Mar 3, 2021

View reviewed changes

Westbrook reviewed Mar 3, 2021

View reviewed changes

adixon-adobe force-pushed the adixon-1235/sp-menu-deprecate-some-usage branch from fdeb60b to 9d6f4f0 Compare March 4, 2021 03:11

adixon-adobe commented Mar 4, 2021

View reviewed changes

adixon-adobe marked this pull request as ready for review March 4, 2021 04:15

adixon-adobe force-pushed the adixon-1235/sp-menu-deprecate-some-usage branch from f6b235b to daac8fe Compare March 4, 2021 04:16

adixon-adobe mentioned this pull request Mar 4, 2021

Overlay should use new reparentChildren helper #1249

Closed

adixon-adobe force-pushed the adixon-1235/sp-menu-deprecate-some-usage branch from a2e592b to be6b177 Compare March 4, 2021 14:55

Westbrook reviewed Mar 9, 2021

View reviewed changes

packages/picker/src/Picker.ts Outdated Show resolved Hide resolved

adixon-adobe added 12 commits March 9, 2021 11:22

feat: deprecate sp-menu in PickerBase derived classes

f7e1ad9

There's still work to update all the tests/docs, but if this is working right most tests should pass.

fix: renamed the file but not this

63f3a96

fix: remove <sp-menu> usage where deprecated

2ac06ae

Also properly import sp-menu for usage in PickerBase

fix: split-button tests & lots of cleanup based on review feedback

bfcc28d

I'd do more on the manageSelection changes here, but I think we can remove a lot of this by using new options from #1189

fix: simplify optionsMenu usage and fix tests

2af9161

fix: slot documentation

f6e4a6a

fix: hopefully fix CI

3918921

fix: analyze type errors, and add deprecated syntax tests

322f5b9

fix: analyze errors, properly this time

d9357a9

fix: missed ActionMenu for type changes

f56ea7d

fix: bad merge conflict resolution

108d6c6

adixon-adobe force-pushed the adixon-1235/sp-menu-deprecate-some-usage branch from 01b0f1d to 108d6c6 Compare March 9, 2021 17:25

Westbrook approved these changes Mar 9, 2021

View reviewed changes

Westbrook merged commit e408d61 into main Mar 9, 2021

Westbrook deleted the adixon-1235/sp-menu-deprecate-some-usage branch March 9, 2021 18:22

Westbrook mentioned this pull request Mar 24, 2021

sp-menu should be deprecated from popover menu classes #1235

Closed

feat: deprecate sp-menu in PickerBase derived classes #1237

feat: deprecate sp-menu in PickerBase derived classes #1237

Uh oh!

Conversation

adixon-adobe commented Mar 2, 2021

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

Uh oh!

adixon-adobe commented Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Westbrook left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Westbrook commented Mar 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adixon-adobe commented Mar 4, 2021

Uh oh!

adixon-adobe commented Mar 4, 2021

Uh oh!

adixon-adobe commented Mar 4, 2021

Uh oh!

Uh oh!

Westbrook left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adixon-adobe commented Mar 2, 2021 •

edited

Loading