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(pagination, split-button, dropdown, date-picker) action-group): add setFocus method #6405

Merged
merged 12 commits into from
Feb 3, 2023
Merged

feat(pagination, split-button, dropdown, date-picker) action-group): add setFocus method #6405

merged 12 commits into from
Feb 3, 2023

Conversation

benelan
Copy link
Member

@benelan benelan commented Feb 1, 2023

Related Issue: #5147

Summary

Adds the public setFocus method to the delegatesFocus components that didn't have it. See #5147 (comment)

@benelan
feat: add setFocus method to interactive components
1c92826
@benelan
Merge remote-tracking branch 'origin/master' into benelan/add-setfocus
d300a3a 
* origin/master:
  1.0.4-next.5
  fix(stepper-item): no longer refer numberingSystem from neighbor stepper component (#6380)
@benelan
test(pagination): add focusable coverage
7fba526
@benelan
test(pagination): add focusable coverage
861c73d
@benelan
Merge branch 'benelan/add-setfocus' of github.com:esri/calcite-compon…
36d6463 
…ents into benelan/add-setfocus

* 'benelan/add-setfocus' of github.com:esri/calcite-components:
  test(pagination): add focusable coverage
@benelan
test(dropdown): add focusable coverage
00cea63
@benelan
test(date-picker): add focusable coverage
68f4086
@benelan
test(action-group): add focusable coverage
d6898c2
@benelan
test(split-button): add focusable coverage
a9e7a75
@benelan
chore: cleanup
60d39ea
@benelan benelan requested a review from a team as a code owner February 1, 2023 01:18
@benelan benelan changed the title feat(pagination, split-button, dropdown, date-picker), action-group: add setFocus method feat(pagination, split-button, dropdown, date-picker) action-group): add setFocus method Feb 1, 2023
@macandcheese
Copy link
Contributor

@geospatialem lets make sure to add the Methods readme component section on the docs site for these when this lands for those components without it currently.

@benelan
Copy link
Member Author

benelan commented Feb 1, 2023 •
edited
Loading

@geospatialem lets make sure to add the Methods readme component section on the docs site for these when this lands for those components without it currently.

It doesn't add/remove sections automatically when stuff changes in docs-json?

@macandcheese
Copy link
Contributor

macandcheese commented Feb 1, 2023 •
edited
Loading

It doesn't add/remove sections automatically when stuff changes in docs-json?

Not anymore... we updated the Gatsby "mdx component" to accept a section prop so we could render heading tags in mdx and have them appended to the table of contents... the previous set up was easier since it rendered only what was available in docs-json, but we couldn't have sub-sections as the heading tags rendered in our Readme component weren't parsed by Gatsby (or, we could and did, but they didn't show up in the table of contents).

jcfranco
jcfranco approved these changes Feb 2, 2023
View reviewed changes
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Awesome!

src/components/date-picker/date-picker.e2e.ts
@@ -209,7 +213,7 @@ describe("calcite-date-picker", () => {
await page.setContent("<calcite-date-picker value='2000-11-27'></calcite-date-picker>");
const date = await page.find("calcite-date-picker");
const changedEvent = await page.spyOnEvent("calciteDatePickerChange");
await date.setProperty("value", "2001-10-28");
date.setProperty("value", "2001-10-28");
Copy link
Member

Choose a reason for hiding this comment

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

Nice cleanup. Could you also add a await page.waitForChanges(); here?

src/components/date-picker/date-picker.tsx
/** Sets focus on the component's first focusable element. */
@Method()
async setFocus(): Promise<void> {
await componentLoaded(this);
Copy link
Member

Choose a reason for hiding this comment

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

Sidebar: maybe we need a new util or to update focusElement to do this for us (wait for load then focus).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that could be nice. Matt started a PR (#6156) to try to cut down the setFocus boilerplate but from our discussion it sounds like we can't use componentOnReady() internally. So your suggestion is a good plan B

src/tests/commonTests.ts Outdated
@@ -213,14 +213,14 @@ export async function focusable(componentTagOrHTML: TagOrHTML, options?: Focusab
const tag = getTag(componentTagOrHTML);
const element = await page.find(tag);
const focusTargetSelector = options?.focusTargetSelector || tag;

await element.callMethod("componentOnReady");
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed? page.waitForChanges() already does this.

@benelan
Merge remote-tracking branch 'origin/master' into benelan/add-setfocus
1d3f53d 
* origin/master:
  docs(input-number, input-text): remove unsupported boolean type from type prop doc (#6394)
  ci(release): lint readme and changelog before committing (#6406)
  ci: allow multiple product team labels (#6411)
@benelan
chore: cleanup
3c62f5b
@benelan benelan added enhancement Issues tied to a new feature or request. pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Feb 3, 2023
@benelan benelan merged commit ceaf532 into master Feb 3, 2023
@benelan benelan deleted the benelan/add-setfocus branch February 3, 2023 05:39
jcfranco pushed a commit that referenced this pull request Feb 4, 2023
@benelan @jcfranco
feat(pagination, split-button, dropdown, date-picker) action-group): …
59de7a5 
…add setFocus method (#6405)

**Related Issue:** #5147

## Summary

Adds the public `setFocus` method to the `delegatesFocus` components
that didn't have it. See
#5147 (comment)
benelan pushed a commit that referenced this pull request Feb 6, 2023
Revert "feat(pagination, split-button, dropdown, date-picker) action-…
3b4bd94 
…group): add setFocus method (#6405)"

This reverts commit ceaf532.
benelan added a commit that referenced this pull request Feb 6, 2023
@benelan
Revert "feat(pagination, split-button, dropdown, date-picker) action-…
0526ac0 
…group): add setFocus method (#6405)"

This reverts commit ceaf532.
benelan added a commit that referenced this pull request Feb 6, 2023
@benelan
revert: feat(pagination, split-button, dropdown, date-picker) action-…
5392549 
…group): add setFocus method (#6405)
benelan added a commit that referenced this pull request Feb 7, 2023
@benelan
revert: "feat(pagination, split-button, dropdown, date-picker) action…
655f2ca 
…-group): add setFocus method (#6405)" (#6426)

This reverts commit ceaf532.


The commit above will be reinstalled in the next few days after we
release a patch.
benelan added a commit that referenced this pull request Feb 8, 2023
@benelan
Merge remote-tracking branch 'origin/master' into benelan/eslint-jsdo…
f2d8185 
…c-rulez

* origin/master:
  docs: update component READMEs (#6352)
  ci(next): fix commit message (#6425)
  fix(vite): resolve lazying loading error in dist build (#6436)
  docs(changelog): remove reverted feature (#6435)
  revert: "feat(pagination, split-button, dropdown, date-picker) action-group): add setFocus method (#6405)" (#6426)
  1.1.0-next.2
  docs(segmented-control): update event description for calciteSegmentedControlChange (#6428)
  fix(tree-item): reverses regression to bring back focus when navigating with keyboard (#6424)
  fix(icon): fix icon normalization to handle x-times-named icons (#6422)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcfranco jcfranco jcfranco approved these changes

Assignees

@benelan benelan

Labels
enhancement Issues tied to a new feature or request. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Milestone
2023 January Priorities Patch - Bugs ...
Development

Successfully merging this pull request may close these issues.
3 participants
@benelan @macandcheese @jcfranco