Skip to content
This repository has been archived by the owner on Jun 12, 2024. It is now read-only.

[info arch] Adds OnThisPage component and aside feature to PageLayout #342

Merged
merged 25 commits into from
Oct 28, 2020

Conversation

katydecorah
Copy link
Contributor

@katydecorah katydecorah commented Oct 16, 2020

This PR merges into #334

  • Create OnThisPage component that displays headings on the page and performs scroll spy to indicate where you are on the page.
    • Replaces react-scrollspy with a local script to detect when a heading is in view.
    • Deletes @mapbox/dr-ui/plugins/create-sections plugin as it's no longer needed.
  • Update PageLayout component.
    • Add aside feature which includes the OnThisPage and Feedback components. On larger devices, the aside is stuck to the right-side of the screen. On smaller devices, OnThisPage moves inline below the page's title and Feedback will appear at the bottom of the page.
  • Update Feedback component.
    • Removed background color and use AsideHeading component to style the component's heading.
  • Update docs-prose.css.
    • You can now use .unprose class on h2 elements to remove the styling.

Note: The dr-ui component home page doesn't have a left sidebar right now. It will once #332 lands.

In addition to the details in the changelog, I wanted to point out a few decisions and get your thoughts:

  • I decided not to use Sticky component (react-stickynode) for the aside feature in PageLayout. I decided to go with a less expensive option, CSS (position: sticky), however this means that the aside feature will not stick on IE 11 and will instead by static. I think this is ok and considered a progressive enhancement as the core functionality of the component of providing anchor links is still attainable.

  • The new OnThisPage component requires that all headings that will be used as links have the anchor class. This will help us to better identify and track via the scroll spy functionality. This should not be a problem for most pages, where this PR updates the add-links-to-headings plugin to add the class to headings automatically. This also allows us to remove (which this PR does) the create-sections plugin which added a lot of divs to sectionize headings for the purpose of scroll spy. Overall, I think this will be an improvement. But during roll out, we'll need to be mindful of pages that use html rather than markdown (like Mapbox GL JS) to make sure we add the anchor class to each h2 and h3 element that is expected to show up in the OnThisPage component.

  • This PR also includes a bunch of small/fiddly updates to the catalog page to enable the Feedback component on all pages to make testing easier.

To do

  • Fix tests
  • Fix aside on catalog page (it scrolls with the page)
  • Add task to [info arch] Redesign PageLayout #332 to increase max-width of limiter class in PageLayout to give the content and new feature enough space.

How to test

  • View the OnThisPage test cases page (npm start)
  • View the catalog site (npm run start-docs) -- Note the homepage of the catalog site doesn't currently have a left sidebar, but that will be changed when the PageLayout updates are added.

QA checklist

  • No errors logged to console.
  • Accessibility score in Chrome DevTools Audit/Lighthouse is 100% with Lighthouse version: #.#.#.
  • Component is accessible at mobile-size viewport.
  • Component is accessible at tablet-size viewport.
  • Component is accessible at laptop-size viewport.
  • Component is accessible at big-monitor-size viewport.
  • Create a PR in a site repo, copy the component, and test it. Push to staging and let the reviewer know they can also test the component there.

Open the test cases app locally on:

  • Chrome, no errors logged to console.
  • Firefox, no errors logged to console.
  • Safari, no errors logged to console.
  • Edge, no errors logged to console.
  • IE11, no errors logged to console.
  • Mobile Safari, no errors logged to console.
  • Android Chrome, no errors logged to console.

Fixes #341

@katydecorah katydecorah mentioned this pull request Oct 16, 2020
4 tasks
@katydecorah katydecorah marked this pull request as ready for review October 20, 2020 14:55
Copy link
Contributor

@danswick danswick left a comment

Choose a reason for hiding this comment

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

This is so good! I tested thoroughly locally and left a few comments (mostly just clarifying) inline.

Regarding the prompts you left:

  • I decided not to use Sticky component... This seems like a great decision to me. The IE trade-off seems perfectly justifiable to me as well.
  • The new OnThisPage component requires that all headings that will be used as links have the anchor class... This also seems like a justifiable decision. Most of this component's users will be working almost exclusively in markdown anyway. If it becomes a big issue, we could consider a custom plugin, but I feel pretty confident we won't need to.

My only concern when looking at the live page is that 1200px might be a bit of a high threshold to sacrifice both a persistent sidebar as well as second-level headings. Right now, anything that takes up less than half the width of my external monitor gets the collapsed version. Maybe we can check back in when the page layout is finalized to see how much real estate we can buy back and see what feels feasible?

src/components/on-this-page/on-this-page.js Outdated Show resolved Hide resolved
{heading.icon && <HeadingIcon name={heading.icon} />}
{heading.value}
</a>
<Headings
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting! I don't think I've seen a recursive component before 👀

text: PropTypes.string.isRequired,
slug: PropTypes.string.isRequired,
icon: PropTypes.string
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think this will ever nest further or are we committed to H2 and H3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we are committed to h2 and h3. I think anything beyond h3 is too granular.

@katydecorah
Copy link
Contributor Author

katydecorah commented Oct 28, 2020

My only concern when looking at the live page is that 1200px might be a bit of a high threshold to sacrifice both a persistent sidebar as well as second-level headings. Right now, anything that takes up less than half the width of my external monitor gets the collapsed version. Maybe we can check back in when the page layout is finalized to see how much real estate we can buy back and see what feels feasible?

@danswick Right, I added a task to #332 to adjust the max-width of the limiter to afford space for the aside

Edit: As I reread your comment, I think you're talking about the media queries and I was thinking about just the limiter!! Yes, you're right. Right now the aside is only sticky starting at -mxl, I agree that once we update the width on the limiter we can try to start sticky for the aside sooner at -ml. I added this to our tracker to remind us to follow up.

@katydecorah katydecorah merged commit 693c73b into info-arch-main Oct 28, 2020
@katydecorah katydecorah deleted the info-arch-aside branch October 28, 2020 14:51
katydecorah pushed a commit that referenced this pull request Dec 1, 2020
* Create placeholder

* Create `Breadcrumb` component (#331)

* Create `Breadcrumb` component

* Update CHANGELOG.md

* Set base meta tag to handle relative asset files

* Clean up

* Add documentation

* Move to utils; add test

* Revert "Set base meta tag to handle relative asset files"

This reverts commit 521218b.

* Add `themeWrapper` prop

* Add missing `key`

* Remove `site`, `subsite`, `section`, and `currentPage` props and replace with `links` array

* Remove `domain` prop; move createUniqueCrumbs to PageLayout utils

* Remove unneeded tests

* Update changelog

* Create OnThisPage component and add aside feature to PageLayout (#342)

* Create OnThisPage component

* Update topbar-sticker.test.js.snap

* Update CHANGELOG.md

* Adjust styling

* Wrap gumshoe in try/catch; fixes tests

* Clean up sentry

* Fix catalog page css

* Add `icon` to example; restyle icons

* Clean up example

* Revert react-stickynode update

* Replace gumshoejs with local code

* Update CHANGELOG.md

* Clean up

* Add themeWrapper prop

* Use number

* Reduce changes

* Fix document

* Update CHANGELOG.md

* Update CHANGELOG.md

* Update CHANGELOG.md

* Update docs-prose.css

* findActiveSection -> isActive

* Wrap OnThisPage in `nav` element; use aria-labelledby to improve a11y

* cite source

* [info arch] Rename ExamplesPage component to HelpPage (#346)

* Rename ExamplesPage -> HelpPage

* Rewrite snapshot

* Add filters to exampleIndex layout (#345)

* begin data selector work for lang, level, product, and videos

* Rename topics to filters batfish helpers; build out filters

* Clean up; add comments

* Move approp image files

* Prevent buttons from shifting

* Update styling

* make sure query value is valid before setting the state

* Comments

* add products, filtering

* Use select

* Add `showFilters` frontMatter prop to define filters the page should show (default is all)

* Move "Getting started" topic to front of topics array

* Add tests

* remove unneeded aria-labelledby

* Create ContentWrapper; move filter to the aside

* Clean up

* Clean up test cases

* Document how filters work

* remove export

* Apply suggestions from code review

Co-authored-by: Colleen McGinnis <colleen.mcginnis@mapbox.com>

* comments

* copyeditor

* export `levels` from LevelIndicator

* comments

* Update batfish-helpers.md

* document the sort order

* Rework HelpPage to accept json to build cards (#347)

* Rework HelpPage to accept json to build cards

* Clean up

* Add tutorial testcase

* Remove introText prop

* Rename file to use kebab-case

Co-authored-by: Katy DeCorah <decorah@mapbox.com>
Co-authored-by: Colleen McGinnis <colleen.mcginnis@mapbox.com>

* Fix filter to work with `topic` and `topics`

* Add columns to filters on smaller screens, col--6 cards, fix showFilters logic (#351)

* PageLayout major structural changes (#344)

* initialize page layout redisign skip precommit

* remove accordion and topbar sticker, initial restructure of page layouts

* refactor sidebar data

* unremove nav configs and clean up some split ends

* address feedback

* update snapshots and navigation test fixture

* fix sub-item issue, reduce cols in sidebar

* remove sidbar configs

* remove superfulous component

* remove extra function

* sticky testing

* remove sticky overrides to bring back the STICK

* update snapshots of course

* Adjust styles; use `nav`; move ProductMenu into sidebar.js

* Combine `accordion` into `navTabs` in navigation helper; make NavAccordion independent

* Add limiter--wide class

* Show feedback on catalog pages

* Use css sticky for sidebar; adjust NavAccordion spacing

* Use flexbox for PageLayout

* Adjust padding on headings (no longer needed due to remove TopBar sticker)

* Move guides out of overview subdir

* More NavAcc styling

* Update CHANGELOG.md

* Styling adjustments

* Multi-structure fixes

* Style and test case adjustments

* Make sidebar scrollable; add hideBreadcrumbs and hideSidebar props

* Add `hideSubpages` option for NavAcc (use case: help)

* Add padding to prevent clipping

* Add OverviewHeader options to PageLayout (#349)

* Allow frontMatter to pass OverviewHeader props; small updates to OverviewHeader

* Update CHANGELOG.md

* Move tag back

* Clean up

* Document overview header

* if there is only one navTab, hide the NavigationAccordion on mobile; combine function

* Fix hasPages logic; set isSinglePageSite

* Remove unused topbar.js and test case; remove `tabListAppend` option

* Update docs

* Fix logic order

* Update example Trub page

* Fix tests

* Reorganize page-layout docs

* Clean up props and documentation; remove "accordion", clean up test cases

* Spacing adjustments

* Apply suggestions from code review

Co-authored-by: Dan Swick <dan.swick@gmail.com>

* Move h1 into flex-child section in OverviewHeader

Co-Authored-By: Dan Swick <2365503+danswick@users.noreply.github.com>

Co-authored-by: Katy DeCorah <decorah@mapbox.com>
Co-authored-by: Katy DeCorah <katydecorah@gmail.com>
Co-authored-by: Dan Swick <2365503+danswick@users.noreply.github.com>

* Make NavigationAccordion toggleable (#350)

* initialize page layout redisign skip precommit

* remove accordion and topbar sticker, initial restructure of page layouts

* refactor sidebar data

* unremove nav configs and clean up some split ends

* address feedback

* update snapshots and navigation test fixture

* fix sub-item issue, reduce cols in sidebar

* remove sidbar configs

* remove superfulous component

* remove extra function

* sticky testing

* remove sticky overrides to bring back the STICK

* update snapshots of course

* Adjust styles; use `nav`; move ProductMenu into sidebar.js

* Combine `accordion` into `navTabs` in navigation helper; make NavAccordion independent

* Add limiter--wide class

* Show feedback on catalog pages

* Use css sticky for sidebar; adjust NavAccordion spacing

* Use flexbox for PageLayout

* Adjust padding on headings (no longer needed due to remove TopBar sticker)

* Move guides out of overview subdir

* More NavAcc styling

* Update CHANGELOG.md

* Styling adjustments

* Multi-structure fixes

* Style and test case adjustments

* Make sidebar scrollable; add hideBreadcrumbs and hideSidebar props

* Add `hideSubpages` option for NavAcc (use case: help)

* Add padding to prevent clipping

* Add OverviewHeader options to PageLayout (#349)

* Allow frontMatter to pass OverviewHeader props; small updates to OverviewHeader

* Update CHANGELOG.md

* Move tag back

* Clean up

* Document overview header

* Make icon a button, seperate from link

* Control toggles in the state

* Clean up

* Comments

* if there is only one navTab, hide the NavigationAccordion on mobile; combine function

* Fix hasPages logic; set isSinglePageSite

* Remove unused topbar.js and test case; remove `tabListAppend` option

* Update docs

* Fix logic order

* Update example Trub page

* Fix tests

* Reorganize page-layout docs

* Clean up props and documentation; remove "accordion", clean up test cases

* Spacing adjustments

* Apply suggestions from code review

Co-authored-by: Dan Swick <dan.swick@gmail.com>

* Move h1 into flex-child section in OverviewHeader

Co-Authored-By: Dan Swick <2365503+danswick@users.noreply.github.com>

* Make icon a button, seperate from link

* Control toggles in the state

* Clean up

* Comments

Co-authored-by: danswick <dan.swick@gmail.com>
Co-authored-by: Dan Swick <2365503+danswick@users.noreply.github.com>

* [Info arch] Add aside with feedback to examples page layout (#353)

* remove examp from layout config and hide bottom feedback on examp pages

* update page layout snapshot

* update changelog

* Rework Tag in ProductMenu (#352)

* Add `small` Tag prop/variant; use small tag in ProductMenu

* Reduce changes

* Reduce div

* Make border conditional

* Move ml-neg3 class out of Tag

* Update src/components/tag/tag.js

* Add small tests cases to themes

* Add theming options to `OverviewHeader` (#358)

* Add theming options to `OverviewHeader`

* `background` -> `theme`

* Update custom.js

* Update overview-header.js

* Filter out pages in filter helper that have no title (#356)

* Update Breadcrumbs on mobile (#355)

* Update Breadcrumb to only show second to last item (with back arrow) on mobile

* Show site name if single structure, section name if multi structure

* show breadcrumbs on mobile if sidebar is hidden

* Add test case

* [Info arch] Add option to override <OnThisPage> default visibilty  (#357)

* add onThisPage frontmatter to override default behavior of aside

* tweak conditionals to make default and non-default behavior clearer

* set onThisPage defaults in layout config

* Create "icon" Tag variant; add Tag to NavigationAccordion (#359)

* Add `small` Tag prop/variant; use small tag in ProductMenu

* Reduce changes

* Reduce div

* Make border conditional

* Move ml-neg3 class out of Tag

* Update src/components/tag/tag.js

* Add `icon` variant to Tag; add tags to NavigationAccordion (small, icon variant)

* Clean up

* Update custom test case to have higher contrast

* [Info arch] Allow appending tabs, including links to external pages, to nav tabs (#354)

* [Info arch] Allow children to appear at the top of example index pages (#360)

* allow children at the top of an example index

* simplify conditional child rearing

* update changelog

* Update Sentry, remark-lint-mapbox

* v2.0.0-beta.0

* [Info arch] Make Card description optional (#368)

* Improve filter and showFilters handling (#362)

* Fix Breadcrumb padding (#361)

* Add "domain" prop to PageShell; remove old search-related props (#363)

* Add "domain" prop to PageShell; remove old search-related props

* Update page-layout.js

* Increase limiter--wide at 800px min-width query (#367)

* IE 11 fixes: replace find with reduce, fix flex-child (#364)

* Replace find with reduce

* Set width: 100% on flex-child to prevent ie11 overflow

* set devBrowserslist: false

* clean up

* Update data.json

* clean up

* v2.0.0-beta.1

* Add "search" filter to ExampleIndex (#369)

* Add "search" filter to ExampleIndex

* Update test case

* v2.0.0-beta.2

* [Info arch] Fix unwanted nav accordion scroll bars in IE (#370)

* Add unprose class to OverviewHeader ul (#372)

* v2.0.0-beta.3

* Add `flex-parent--center-cross` to OverviewHeader to center content (image) vertically (#375)

* v2.0.0-beta.4

* Improve color contrast on `LevelIndicator` (#366)

* Add padding around catalog site

* [info arch] Add Search to PageLayout sidebar (#377)

* Add Search to PageLayout sidebar

* Add "Guides" to titleGenerator as a title to not push

* Deprecate `GlossaryCard`, `GlossaryPage`, and `GlossarySection` components (#376)

* v2.0.0-beta.5

* Remove unused packages

* Fix path to file to work on test cases app and catalog page (#379)

* Set height to prevent content shift as Search component loads (#380)

Co-authored-by: Dan Swick <dan.swick@gmail.com>
Co-authored-by: Colleen McGinnis <colleen.mcginnis@mapbox.com>
Co-authored-by: Dan Swick <2365503+danswick@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants