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.

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

[calcite-flow-item] does not have a back button if within a shadow root #6237

Closed
ethanbdev opened this issue Jan 5, 2023 · 23 comments
Closed
Assignees
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Map Viewer Issues logged by ArcGIS Map Viewer team members. enhancement Issues tied to a new feature or request. has workaround Issues have a workaround available in the meantime. p - medium Issue is non core or affecting less that 60% of people using the library

Comments

@ethanbdev
Copy link
Contributor

Actual Behavior

With showBackButton being internal and defaulting to false, by default a flow item within a shadow root is missing the back button, breaking the functionality of a flow item.

Expected Behavior

It should show a back button when it is the active flow item

Reproduction Sample

https://codepen.io/eborgen/pen/vYaygZy

Reproduction Steps

Observe that with a structure that puts a shadow root boundary between the calcite-flow and calcite-flow-item, the back button does not render.

Reproduction Version

next.707

Relevant Info

No response

Regression?

between beta.99 and next.707

Impact

High - it breaks the workflow unless I set an internal prop and ignore the type error

Esri team

ArcGIS Map Viewer

@ethanbdev ethanbdev added bug Bug reports for broken functionality. Issues should include a reproduction of the bug. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Jan 5, 2023
@github-actions github-actions bot added the ArcGIS Map Viewer Issues logged by ArcGIS Map Viewer team members. label Jan 5, 2023
@geospatialem geospatialem added this to the 2023 January Priorities milestone Jan 5, 2023
@geospatialem geospatialem removed the needs triage Planning workflow - pending design/dev review. label Jan 5, 2023
@geospatialem
Copy link
Member

Could be related to the utilities used, will need a refactor in the component to mitigate without the use of the utility functions.

@jcfranco
Copy link
Member

@ethanbdev I could be misinterpreting the issue, but from what I can recall, flow never supported flow-items within the shadow DOM of extraneous components. Can you update your codepen or provide one that shows this behavior?

@ethanbdev
Copy link
Contributor Author

@ethanbdev I could be misinterpreting the issue, but from what I can recall, flow never supported flow-items within the shadow DOM of extraneous components. Can you update your codepen or provide one that shows this behavior?

That makes sense, I think we are not using the flow-item quite correctly. But with the removal of the showBackButton attribute it made it not work at all. It could be reasonable to have a tree structure like:

<calcite-flow>
  <my-custom-component>
    <calcite-flow-item />
  </my-custom-component>
</calcite-flow>

If that is not supported we can adjust the structure or consider a different component, like a panel with a slotted action in the header.

@geospatialem
Copy link
Member

Allocating to the February release for further discussion.

@jcfranco
Copy link
Member

@ethanbdev Gotcha. In the above structure snippet, is the item in the same light DOM tree as the flow? If so, I think that would work. I believe Map Viewer has some components structured similarly. @AdelheidF can you confirm?

@AdelheidF
Copy link

AdelheidF commented Jan 26, 2023

Yes, I use this in a few places, though with

shadow: false,
scoped: true

image
image

@ethanbdev
Copy link
Contributor Author

@ethanbdev Gotcha. In the above structure snippet, is the item in the same light DOM tree as the flow? If so, I think that would work. I believe Map Viewer has some components structured similarly. @AdelheidF can you confirm?

It is not, all of our components utilize shadow: true

image
(I added show-back-button manually, ignoring the type error)

@alisonailea alisonailea changed the title calcite-flow-item does not have a back button if within a shadow root [calcite-flow-item] does not have a back button if within a shadow root Feb 15, 2023
@jcfranco
Copy link
Member

jcfranco commented Mar 4, 2023

@ethanbdev Thanks for the info and apologies for the belated reply. 😅 We'll need to revisit the timeline for this while we do some research on the approach. FWIW, we prototyped something that could be used as the base for this: #4817 (comment).

Did you find a workaround or is this still a high impact issue for you?

@AdelheidF
Copy link

I would very much like to use shadow: true as well. Considering that we want to create all these custom flow-item wrapping components to be used everywhere they all run into this issue. I would still like to see this fixed.

If I don't use shadow: false the flow-items appear in a single panel underneath each other with 1.0.7.

@ethanbdev
Copy link
Contributor Author

I would say it has moved to a more moderate impact. The workaround is to use an undocumented, internal prop. We hope to change the structure of the components to not have the flow item in the shadow root, but I think it still felt like a regression when this behaviour changed

@johngalambos
Copy link

It's possible I'm doing things wrong here, but with the calcite-components-react, I think the the flow item is always in a shadow root? And without the showBackButton attribute public anymore, I don't think it's possible to display a back button?

E.g.
`function AreaSelector (props:NoSelectionProps) {

return (
	<CalciteFlow>
	<CalciteFlowItem
		heading="Areas"
		closed={false}>
		<div className="esri-widget esri-widget--panel parcel-manager parcel-manager__panel-content">
			<div className="parcel-manager__panel-content__message">Select an area</div>
		</div>
	</CalciteFlowItem>
	</CalciteFlow>
);

}
`

renders as:
image

@driskull
Copy link
Member

driskull commented Apr 28, 2023

I think we want to make sure parent/child components are within the same DOM and not separated by a shadowDOM so that they can work correctly.

I think even native elements need to be like this to work and are accessible.

I may be mistaken, but I don't think you can do a native select and child option elements in a shadowRoot.

@jcfranco jcfranco added 2 - in development Issues that are actively being worked on. and removed research Issues that require additional research in order to resolve or make decision. 1 - assigned Issues that are assigned to a sprint and a team member. labels Aug 31, 2023
jcfranco added a commit that referenced this issue Sep 1, 2023
**Related Issue:** #6237 

## Summary

This enables `flow` to work with custom components using shadow DOM
wrapping `flow-item`.

Custom components will have to implement the
[`FlowItemLike`](https://github.com/Esri/calcite-design-system/pull/7608/files#diff-82c222ab365cde13a1f1288d936611519dfd9bee1e283164b260ca554c04a191R3-R7)
interface and set the new `custom-item-selectors`/`customItemSelectors`
attr/prop to target custom components (see [E2E
test](https://github.com/Esri/calcite-design-system/pull/7608/files#diff-9b86e64de24dfca441533c63ae0f6834bff10bffbde23fd8bb3989a2259e356cR315-R387)
for vanilla JS example).

**Note**: `customItemSelectors` is intentionally marked internal since
we don't have any documentation yet on developing custom components,
which this is meant to support. We could additionally hide
`FlowItemLike` in the doc site to avoid confusion until we have more
documentation on this use case. I'm open to suggestions on this.
@geospatialem @macandcheese @driskull
@jcfranco jcfranco added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Sep 1, 2023
@github-actions github-actions bot assigned geospatialem and unassigned jcfranco Sep 1, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2023

Installed and assigned for verification.

@jcfranco
Copy link
Member

jcfranco commented Sep 1, 2023

@ethanbdev @AdelheidF calcite-flow now supports custom flow items (i.e., custom elements using shadow DOM that wrap a calcite-flow-item). There are 2 steps needed to have flow pick these custom items:

  1. Set flow's customItemSelectors/custom-item-selectors to match the custom item element.
  2. Update your custom item element to implement the FlowItemLike interface. The simplest approach would be to wrap a flow item, which I believe you already do.

Here's an example showing how you can do that with vanilla JS: https://codepen.io/jcfranco/pen/zYyBJEV. It should be much more straightforward w/ Stencil or similar tooling.

Stencil component example
import { Component, Host, h, Prop, Method } from "@stencil/core";
import type { FlowItemLike } from "@esri/calcite-components/dist/types/components/flow/interfaces.d.ts";

@Component({
  tag: 'custom-flow-item',
  shadow: true,
})
export class CustomFlowItem implements FlowItemLike {

  private flowItem: HTMLCalciteFlowItemElement;

  @Prop() beforeBack: () => Promise<void>;

  // not required by the interface, but adding to allow passing through to `calcite-flow-item`
  @Prop() heading: string;

  @Prop() menuOpen: boolean;

  @Prop() showBackButton: boolean;

  @Method()  async setFocus(): Promise<void> {
    await this.flowItem.setFocus();
  };

  render() {
    return (
      <Host>
        <calcite-flow-item
          beforeBack={this.beforeBack}
          heading={this.heading}
          menuOpen={this.menuOpen}

          // @ts-ignore -- internal
          showBackButton={this.showBackButton}
     
          // eslint-disable-next-line react/jsx-sort-props -- ref should be last so node attrs/props are in sync (see https://github.com/Esri/calcite-design-system/pull/6530)
          ref={el => this.flowItem = el}
        >
          <slot/>
        </calcite-flow-item>
      </Host>
    );
  }
}

Note that the new prop and interface won't be available in the documentation until we have something covering custom element creation (no timeline on that). cc @geospatialem @macandcheese

Let us know if you need any help or have any questions.

Happy coding! ✨💻✨

@geospatialem geospatialem added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Sep 1, 2023
@geospatialem
Copy link
Member

Verified with the Codepen mentioned in Franco's post above. ☝🏻

@AdelheidF
Copy link

AdelheidF commented Sep 2, 2023

The & HTMLElement is giving me issues here
export type FlowItemLikeElement = Pick<HTMLCalciteFlowItemElement, "beforeBack" | "menuOpen" | "setFocus" | "showBackButton"> & HTMLElement;

Error
Class 'ArcgisSmartMappingGallery' incorrectly implements interface 'Pick<HTMLCalciteFlowItemElement, "beforeBack" | "menuOpen" | "setFocus" | "showBackButton"> & HTMLElement'. Type 'ArcgisSmartMappingGallery' is not assignable to type 'HTMLElement'. Type 'ArcgisSmartMappingGallery' is missing the following properties from type 'HTMLElement': accessKey, accessKeyLabel, autocapitalize, dir, and 285 more.ts(2420)

@AdelheidF AdelheidF reopened this Sep 2, 2023
@jcfranco
Copy link
Member

jcfranco commented Sep 2, 2023

#7666 will split up the interface into:

  • FlowItemLike – interface for classes to implement
  • FlowItemLikeElement – interface for APIs of parent components supporting custom items

I've also added a Stencil-based example for custom flow items in my previous comment.

Technically, the custom item doesn't need to implement the interface for it to be picked up by flow, but it should to ensure APIs are in sync.

@jcfranco jcfranco added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 4 - verified Issues that have been released and confirmed resolved. labels Sep 5, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2023

Installed and assigned for verification.

@jcfranco
Copy link
Member

jcfranco commented Sep 5, 2023

#7666 landed! 🛬

@AdelheidF Would you be able to help verify once 1.7.1-next.3 (or greater) is deployed later today?

@AdelheidF
Copy link

Seems good with 1.8.0-next.0. Thanks!

@geospatialem geospatialem added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Sep 5, 2023
@geospatialem
Copy link
Member

Closing per #6237 (comment).

@jcfranco
Copy link
Member

jcfranco commented Sep 6, 2023

@AdelheidF Thanks for testing! 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Map Viewer Issues logged by ArcGIS Map Viewer team members. enhancement Issues tied to a new feature or request. has workaround Issues have a workaround available in the meantime. p - medium Issue is non core or affecting less that 60% of people using the library
Projects
None yet
Development

No branches or pull requests

8 participants