`Breadcrumb` and page title component

SimonFinney commented 4 years ago

Description

Provide a generic component for the combined Breadcrumb and page title pattern, including animations, seen in CPFS for reuse across the business unit:

breadcrumb-interaction

The originating source is currently available in the shell repo under src/browser/breadcrumbs and needs to be converted for use as a React component.

Acceptance criteria

A consuming developer can

[ ] supply a title to be displayed for the page
[ ] supply a path for the root / 'Homepage'
[ ] supply a list of paths for subsequent breadcrumb items
[ ] choose whether to display the root / 'Homepage' or not

Other

[ ] The title will animate into the breadcrumbs when the viewport is scrolled

Documentation

The most current specs, plus the best description of breadcrumbs as historical/hierarchical: https://ibm.box.com/s/7gz5f2p1nwk0vd9gmk15vg6ra4r0ii9u Older documentation but does demonstrate the interaction: https://github.ibm.com/security/isc-patterns/blob/master/components/breadcrumbs/readme.md

jendowns commented 4 years ago

After reviewing this request and the reference component, I don't believe that component, as defined, is suitable for this library.

Consider the following details about the reference component from the "shell" repo:

Does not separate concerns in an app-agnostic way
Manages a scroll listener
Manages a Portal to render a title node into the root element

And then consider each of these details separately...

Separation of concerns

There's a fuzzy boundary between library concerns and app concerns. But to me the reference component feels firmly in the "app" territory. When I think about how to genericize its functionality I have a strong sense I'm writing a component that "knows too much":

It is modifying the breadcrumb items
It is listening for scroll events on the window
It is taking control of the <h1> element for the current page of the app

Of course we can port any & all of that functionality, but at the expense of having to make some unusual DOM manipulations and event handlers. As a user of our library I don't think I would want it to be doing these types of manipulations (especially as we look toward the "bedrock" component library and what that means in the future).

Manages a scroll listener

Generally, we should be very hesitant to use scroll listeners in our library due to the negative performance impact.

I also recommend that, as a component library, we should avoid attaching anything (events, properties) to the window whenever possible.

Manages a Portal to render a title node into the root element

I also mentioned this in the "separation of concerns" section but I wanted to call it out specifically:

The reference component is taking control of the <h1> of an app's page. It uses a portal to render the title node into root element. This is a higher-level or even app-level action that should not be taken in a generic component from our library.

Next steps

I believe this request needs to be re-examined from the perspective of what makes sense for our library vs. a client's app code.

Thinking on this, I think a piece that would be appropriate for our library is the animation of breadcrumb items. I could see a case for a component that holds breadcrumb items in state. And when a breadcrumb item is added or removed, then it is animated to appear or disappear from the list of breadcrumbs. That component would only know what you pass it, and what it saves in state.

SimonFinney commented 4 years ago

Thanks for the write-up @jendowns - I appreciate you taking the time to share your thoughts here, and I'd love to see some of the experimentation completed as part of this to better understand the challenges you've outlined above.

I'd love to hear your thoughts on how we can produce something that delivers the desired visual consistency for this pattern while mitigating the DOM manipulation concerns you've discussed above. Currently, the onus is on anyone wanting to adopt this pattern to implement it themselves, and that is creating fragmentation across IBM Security, and will continue to across Cloud Paks if it's not appropriately standardized on.

jendowns commented 4 years ago

@SimonFinney

I'd love to see some of the experimentation completed as part of this...

I'm not sure what you mean here. I have not ported any of the DOM manipulation or scroll listening, for reasons above.

I'd love to hear your thoughts on how we can produce something that delivers the desired visual consistency for this pattern while mitigating the DOM manipulation concerns you've discussed above.

I answered some of that in the "Next Steps" section at the bottom of my response. In that section, I mentioned a piece of this request that makes sense for this library. But the other pieces of the reference component, as is, are app-level logic that I don't believe make sense for this library. So to mitigate them would be to remove them/not add them, leaving the suggestion I made in the "Next Steps" section.

EDIT: I didn't finish responding here, but I think the next step would be to look at the referenced component in the other repo and revisit the scroll listener and DOM manipulation there. I think a scroll listener could be replaced with something like an intersection observer, if some structure changed a bit & maybe also there was as common parent component to use (or maybe an exported utility?). And for the DOM manipulation, I don't think it's truly necessary -- the same "page heading" could remain in the app as an h1 and also passed to the breadcrumbs. One doesn't need to interact with / change the other.

Currently, the onus is on anyone wanting to adopt this pattern to implement it themselves, and that is creating fragmentation across IBM Security, and will continue to across Cloud Paks if it's not appropriately standardized on.

The reference component from the "shell" repo is currently in use, right?

stale[bot] commented 3 years ago

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

SimonFinney commented 3 years ago

Superseded by https://github.com/carbon-design-system/ibm-cloud-cognitive/issues/59

carbon-design-system / ibm-security