Add ScrollSpy component

goldylucks commented 5 years ago

[x] This is not a v0.x issue.
[x] I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

Like zurb's Magellan. highlight navbar item based on scroll and/or based if element is in viewport

Examples 🌈

See link above

Context 🔦

I would like to create this component for MUI! I'm happy to submit a PR with some guidance :)

Benchmark

brianle1301 commented 5 years ago

Isn't it what Material-UI already offered?

oliviertassinari commented 5 years ago

@dreamsinspace Right, the documentation implements it internally. The source: https://github.com/mui-org/material-ui/blob/404c2ba16816f5c7ab7d8b2caf6bbc3d2218b820/docs/src/modules/components/AppTableOfContents.js.

@goldylucks Do you have another example of UI library implementing it? Ant calls it Anchor. We call it TableOfContents, Foundation calls it Magellan. What's the proper name for this component?

samuelcolburn commented 5 years ago

I'm not convinced this really needs to be a framework component.

Regardless, I think Anchor or something similar would be the most appropriate name. To me it is very reminiscent of native <a href="#section"> elements and their scroll-to functionality.

goldylucks commented 5 years ago

@samuelcolburn the idea here is to sync the active link with the scroll position, not just render an anchor element :)

@oliviertassinari, I've done a short search and here are some related resources:

zurb's Magellan
react-scrollspy - this is what I'm currently using
medium article "Scrollspy"
Scrollspy pen with multiple images
react-scroll-agent
mdbootstrap-scrollspy
Antd anchor

Seems like scrollspy is winning the race here ;)

I personally like ScrollSpy. Sounds more intuitive to me then TableOfContents.

Regardless, in the docs, we can add a list of all known names of the component, so no matter what future MUI consumers will search, they will find it. Something like ...

"ScrollSpy is ... It's also known as TableOfContents, Magellan, StickyScrollNav"

I think it's a good idea to treat discoverability as the leading motivation here.

Thoughts?

btw, is TableOfContents ready for production? if so, any reason it's not in the docs? I can add it with some guidance :)

brianle1301 commented 5 years ago

@goldylucks We could even have different aliases of <ScrollSpy />, although I'm not sure if this has any drawbacks

oliviertassinari commented 5 years ago

@goldylucks Thanks for sharing this benchmark. I think that it's a low priority component for us, given few people actually seem to need (based on the downloads of the linked packages). I don't think that it's something we should consider in the near future. However, given it's something we have already implemented in the documentation, it could be a low hanging fruit. The biggest challenge here might be to agree on what the API should be. It's worth exploring.

react-scrollspy - this is what I'm currently using

What's wrong with this one? Why would you want to switch? (The API looks great, btw)

mbrookes commented 5 years ago

However, given it's something we have already implemented in the documentation, it could be a low hanging fruit.

It could possibly move into the @material-ui/docs package.

samuelcolburn commented 5 years ago

@samuelcolburn the idea here is to sync the active link with the scroll position, not just render an anchor element :)

@goldylucks I recognize that. The idea though is to essentially have an anchor list that highlights your current position, so having the component name include or reference anchor in some way makes sense to me.

However, if there is already some naming convention around these types of components like ScrollSpy or TableOfContents then that's is fine too. I would add though that TableOfContents sounds more like a component, whereas I think ScrollSpy is more indicative of a reusable hook like useScrollSpy.

goldylucks commented 5 years ago

@oliviertassinari

I think that it's a low priority component for us, given few people actually seem to need (based on the downloads of the linked packages).

The download number represents the people who found it ;) we could have missed many people who turned elsewhere for solutions (like yours truly here ...)

However, given it's something we have already implemented in the documentation, it could be a low hanging fruit.

So it's documented but not implemented? Is that the case? I'm happy to prepare a PR for this, A-Z, so you (or whoever is responsible) would only need to code review and ask for changes if needed.

We can also take a different approach here.

I can implement this as an independent npm package, material-ui-scroll-spy, following MUI's API approach.

If it gains traction, we can revisit this and think about merging it into core.

I'm good with both approaches :)

What's wrong with this one? Why would you want to switch? (The API looks great, btw)

The API indeed is great, and I'll probably ~steal~ get heavily inspired from it. But ... it doesn't have MUI "baked in", so it's not an out of the box full solution.

goldylucks commented 5 years ago

@mbrookes

Isn't it already in the docs like @oliviertassinari linked above?

goldylucks commented 5 years ago

@samuelcolburn

@goldylucks I recognize that. The idea though is to essentially have an anchor list that highlights your current position, so having the component name include or reference anchor in some way makes sense to me.

Right! Makes sense to me as well, sorry for the misunderstanding.

I would add though that TableOfContents sounds more like a component, whereas I think ScrollSpy is more indicative of a reusable hook like useScrollSpy.

Scroll spy doesn't sound intuitive to me either. when I searched it last week, I googled queries along the lines of:

react attached scroll nav
react scroll nav
react nav scroll
react scroll menu
react sticky scroll

After not finding anything, I went to zurb's foundation site and browsed their components, and found magellan (their implementation of this component).

Then I went to bootstrap and bowsed their components, came out empty handed.

From the short research I posted above, seems like variations on the words "scroll" and "spy" are the most common for this component/behavior.

I don't know what's the flow you guys have to decide on a new component, and I think it would make sense to first agree on whether or not we want this component, and then have a separate discussion for namings and API.

Maybe you guys have a more evolved "protocol" than this of course.

btw I didn't mention it before, MUI is AMAZING! Saved me many hours and "white hairs" (local expression)

oliviertassinari commented 5 years ago

Then I went to bootstrap and bowsed their components, came out empty handed.

@goldylucks Here you go: https://getbootstrap.com/docs/4.3/components/scrollspy/

I don't know what's the flow you guys have to decide on a new component, and I think it would make sense to first agree on whether or not we want this component, and then have a separate discussion for namings and API.

I have, personally, no objection with trying this new component out in the lab.

goldylucks commented 5 years ago

@goldylucks Here you go: https://getbootstrap.com/docs/4.3/components/scrollspy/

I guess I missed that @oliviertassinari !

I have, personally, no objection with trying this new component out in the lab.

Great. Do we need other people to confirm?

I've read the contributing guide, let me know if/when to start working on that

oliviertassinari commented 5 years ago

@goldylucks we use the lab as a playground to try things out. We add, remove, break things as the different iterations require it. The first step would be to open a pull request under it. Then, we will review it.

Matt was proposing to host the component in the docs package. Why not. I believe that the final location should be discussed after we have run our first experimentation in the lab and was proven to be successful.

mbrookes commented 5 years ago

It could possibly move into the @material-ui/docs package.

Isn't it already in the docs

@goldylucks Yes, the TOC component is used in the docs, but isn't in the @material-ui/docs package, which is where we expose some of the docs site internals for public use.

ahallora commented 5 years ago

I made a crude rewrite of the component from MUI Docs that supports my needs. Perhaps it will support yours as well? 😄

Here's the link to my react component which integrates the scrollspy-component from MUI Docs with a MUI Tab-component, named scrollSpyTabs (naming is hard): https://codesandbox.io/s/material-demo-xu80m

And just for inspiration and, if you for some reason don't want a react component, here's a rewrite in plain javascript of user rishabhp's jQuery take: https://codepen.io/anon/pen/JgXLaQ?editors=0010

oliviertassinari commented 5 years ago

@ahallora Thanks for sharing

goldylucks commented 5 years ago

@ahallora that's cool!

What do you think about the following syntax?

        <ScrollSpy>
          <ScrollSpyTabs>
            <ScrollSpyTab />
            <ScrollSpyTab />
          </ScrollSpyTabs>
          <ScrollSpyContents>
            <ScrollSpyContent />
            <ScrollSpyContent />
          </ScrollSpyContents>
        </ScrollSpy>

I think it's more inline with MUI's conventions.

Altho it would be extra nice if the content and the tabs will be decoupled. That could be accomplished by context, but we can leave that for v2 :)

ahallora commented 5 years ago

You are more than welcome to give it a shot! 😉

My goal was to just get it working standalone. As I'm still quite new to React (and MUI) the code obviously requires a lot more polishing.

Here's a repo on github so you can fork and improve, if you want to: https://github.com/ahallora/scrollSpyTabs 🤓

oliviertassinari commented 5 years ago

@goldylucks What do you think of the API of react-scrollspy?

goldylucks commented 5 years ago

Their API is great! I thought about using it under the hood for MUI's scroll spy.

And I think we can make a better API, more out-of-the-box style.

What do you think of the following API?

        <ScrollSpy>
          <ScrollSpyTabs>
            <ScrollSpyTab />
            <ScrollSpyTab />
          </ScrollSpyTabs>
          <ScrollSpyContents>
            <ScrollSpyContent />
            <ScrollSpyContent />
          </ScrollSpyContents>
        </ScrollSpy>

The main benefit I see here is it requires ZERO configuration/props. You simply put some tabs, put some ccontent, and it just works.

The ScrollSpy parent component assigns the ref's sequentially by default, so the user don't have to manually write hrefs and ids anywhere.

And we can extend the functionality by exposing a context, to allow the tabs and contens to be placed in different parts of the dom and not couple them to the same parent.

What do you think? Shall I make a POC?

oliviertassinari commented 5 years ago

What do you think of the following API?

@goldylucks I think that we should aim for a low-level API, to start with. A success criterion for the first iteration would be that we can use the logic for the table of contents in the documentation. Then we can consider an higher-level API.

I think that no matter what, it will be better to expose the active state so people can play with it. For instance, the user might want to commit the value to the URL, I know @mbrookes first implemented the TOCs like this, so there is an audience for it.

I'm wondering if we couldn't start with a useScrollSpy hook:

import { useScrollSpy } from '@material-ui/lab';

export default function Header() {
  const active = useScrollSpy({
    items: ['#section-1', '#section-2', '#section-3'],
    target: window,
  });

  return <Tabs value={active} />;
}

We would most likely be able to use useScrollTrigger internal going down this path. It would be about providing a custom getTrigger function option.

Alternatively, I find the selector option of https://github.com/manifoldco/react-scroll-agent very elegant 😍, we could support both:

<ScrollAgent
  nav={({ current, positions }) => (
    <menu>
      <a href="#section-1" className={current === 0 ? 'is-active' : ''}>
        Section 1
      </a>
      <a href="#section-2" className={current === 1 ? 'is-active' : ''}>
        Section 2
      </a>
      <a
        onClick={() => window.scrollTo(0, positions[2])}
        className={current === 2 ? 'is-active' : ''}
      >
        Section 3
      </a>
    </menu>
  )}
  selector="section[data-scrollspy]"
  threshold="center"
>
  <section id="section-1" data-scrollspy>
    <h1>Section 1</h1>
  </section>
  <section id="section-2" data-scrollspy>
    <h1>Section 2</h1>
  </section>
  <section id="section-3" data-scrollspy>
    <h1>Section 3</h1>
  </section>
</ScrollAgent>

so the user don't have to manually write hrefs and ids anywhere.

Hum, I'm wondering if we shouldn't force the usage of ids and hrefs, to improve the UX and probably the a11y too.

The points I'm unsure about:

Should items accept a string selector or/and a DOM node or/and a href?
Should the returned item be a DOM node or/and a match index or/and a href?

iDVB commented 4 years ago

I created a watered down custom hook that works the same as MUI's AppTableOfContents.js.:

https://gist.github.com/iDVB/a041da210605f05e0b36ac03ed403c00

import React from 'react';
import _findIndex from 'lodash.findindex';

const items = [
  { hash: "breaking-news", label: "Breaking News" },
  { hash: "foundation", label: "Foundation" },
  { hash: "collaborations", label: "Collaborations" },
  { hash: "playbooks", label: "Playbooks" },
  { hash: "deploying-digital", label: "Deploying Digital" }
];

const NavBar = props => {
  const active = useScrollSpy({ items });
  const activeIndex = active ? _findIndex(items, ["hash", active]) : false;

  const tabsHTML = items.map(({ label }, index) => (
    <Tab
      key={index}
      label={label}
    />
  ));

  return (
    <Tabs value={activeIndex}>
      {tabsHTML}
    </Tabs>
  );
};

vishalrajole commented 3 years ago

Thanks for this great progress so far. I indeed would say, this is a nice to have feature. I am searching for same with Material UI inbuilt/officially supported for vertical tabs. But since it is not official yet, I had to convince UX team for time/efforts required as it is not from library itself and then we dropped it off.

IMO, when exploring Material UI docs, UX people check for components available & then decides what components/features we can leverage. So until any component/feature is available with stable version, it will show low downloads as it is not available & developers will just search for other ways.

I am looking forward with this feature in stable version. I am already using v5.0.00-alpha.33. @oliviertassinari

mui / material-ui