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

[EuiTour] Allow attaching tour steps via HTMLElement or DOM selector #5696

Merged
merged 19 commits into from
Mar 17, 2022

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Mar 7, 2022

Summary

Closes #5576.

Adds an anchor prop to EuiTourStep that accepts an HTMLElement or function returning an HTMLElement, or a DOM querySelector.

  • Add anchor prop to EuiTourStep
    • Use anchor instead of supplying children
    • EuiTourStep will not wait for the element to exist before attempting to render
      • Consumers should maintain logic around the state of their UI
      • It'll wait a tick on mount using window.requestAnimationFrame
  • Create a shared findElement service

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5696/

@thompsongl
Copy link
Contributor Author

👋 @kertal @Heenawter @andreadelrio

I was told that y'all would be interested in these changes. Before I formalize testing and documentation, does it seem like this addition will solve your case for EuiTour?

@Heenawter
Copy link
Contributor

@thompsongl Wow, this looks great!! Thanks so much for working on it :) I'm going to take some time today to test this locally and see what I can do with it - but from a code review only, it looks like exactly what we were looking for!

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5696/

@Heenawter
Copy link
Contributor

Heenawter commented Mar 8, 2022

@thompsongl So far, it's definitely a whole lot easier to use!! I was able to add a two step tour to Discover with minimal effort to elements that I was unable to even target before, which is awesome! :)

2022-03-08_ShortDiscoverTour.mp4

A few notes/ideas of things that would make it even easier to use:

  • Would it be possible to have default Next behaviour added to the bottom of the tour steps? Right now I have to pass a custom footer action to each step except the final one, which is a bit clunky - I understand that, in the examples included in the documentation, the tour steps progress via actions such as copy pasting and clicking other buttons rather than clicking Next; however, I think that a Next button would, at the very least, be a common use case worth considering. Here is an example of what we had in our original tour for what was considered the 'default' footerAction:

    <EuiFlexGroup responsive={false} gutterSize="s" alignItems="center">
      <EuiFlexItem grow={false}>
        <EuiButtonEmpty color="text" size="xs" onClick={actions.finishTour()}>
          Skip tour
        </EuiButtonEmpty>
      </EuiFlexItem>
      <EuiFlexItem grow={false}>
        <EuiButton size="s" color="success" onClick={actions.incrementStep()}>
          Next
        </EuiButton>
      </EuiFlexItem>
    </EuiFlexGroup>

    Which rendered to this:

    image

  • Could there be an easier way of injecting all the tour steps in to one spot now? In the video above, I ended up making a new DiscoverGridTour component that returns the following:

    return (
        <>
          <EuiTourStep
            {...euiTourStepOne}
            footerAction={
              <EuiButtonEmpty color="text" flush="right" size="xs" onClick={actions.incrementStep}>
                Next
              </EuiButtonEmpty>
            }
          />
          <EuiTourStep {...euiTourStepTwo} />
        </>
      );

    Then, right above the DiscoverGrid component in discover_grid.tsx I was able to inject these tour steps using the following:

    ...
    return (
        <>
          <DiscoverGridTour /> // <--- Injecting all the steps right here
          <DiscoverGridContext.Provider
            value={{
              expanded: expandedDoc,
    ... 

    We probably don't want tours with more than a handful of steps anyway, but having to manually spread each EuiTourStepProp is a bit clunky - this could be done with a loop or something to make it smaller, but perhaps worth considering another option entirely? Similar to how the EuiTour component works in the "Managed state via EuiTour render prop component" documentation example, but without having to spread each step. Totally possible this already exists and I missed it, though :)

This is so exciting!! I'm stoked to see where this goes 🔥

@thompsongl
Copy link
Contributor Author

@Heenawter Happy to see that this helps! Thanks for awesome feedback, also.

What I'll do is 1) round out the DOM selector feature in this PR and 2) open issues/PRs for your suggestions. I'd like to think through options for a single injection point (á la <DiscoverGridTour />) and other implications of needing to rely less on markup composition.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5696/

@thompsongl thompsongl marked this pull request as ready for review March 14, 2022 21:27
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5696/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5696/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5696/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5696/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5696/

Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

I have a some optional unit test feedback, but otherwise this LGTM!

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5696/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiTour] Allow attaching tour steps via DOM selector
5 participants