reapit / elements

Reapit Elements UI Component Library
https://elements.reapit.cloud/?path=/story/welcome--page
0 stars 2 forks source link

v5 component refactor: App Bar #153

Open HollyJoyPhillips opened 4 weeks ago

HollyJoyPhillips commented 4 weeks ago

Abstract

As part of the v5 Elements release, each component will be reviewed and refactored to ensure best practice and design system alignment

Specification

Developer Checklist

Release Checklist

Additional Context or Information

jmatiushina commented 3 weeks ago

Pointing out the issue I've had as if it might be relevant to the task and helpful for who ever picks it up. I'm passing in brandOptions:

brandOptions={{
            callback: openNewPage('agencycloud://process/webpage?url=https://mysite.com'),
            logoUrl: 'https://uk.payprop.com/res/assets/img/pp_logo.svg',
          }}

As you can see I'm trying to redirect to the webpage outside of the agency cloud. Open new page uses window.open which I thought would solve my issue.

export const openNewPage = (uri: string) => () => {
  window.open(uri, '_blank')
}

In the use-case described above my logo is not clickable within the agency cloud and redirect isn't happening. I have checked the link itself and the issue disappears if I use an href.

<a href="agencycloud://process/webpage?url=https://mysite.com">My site</a>

Within current component there is no href option. Maybe we can add it with v5 refactor? cc @HollyJoyPhillips

kurtdoherty commented 2 weeks ago

suggestion: @adrian-reapit Like a few of the other v5 tickets currently on the board that I've commented on, I think this is another one that needs to move away from a config-first component API to a composition-first one. What I mean by this is, instead of trying to facilitate all possible configurations of the app bar and the various pieces involved in it within a single prop interface, we really need to lean into composing separate components together via JSX, as that's "the React way".

By composition, I mean something like:

<GlobalAppBar
  brand={<MyBrandLogo />}
  nav={<MyTopNav />}
  utilities={<MyUtilityIcons />}
/>

Here, the GlobalAppBar component is really just handling the layout of the other "slots". Consumers would then compose other Design System components into the appropriate slots, but they have the flexibility to configure them as necessary without GlobalAppBar having to explicitly support all those other component's props as well.

aside: I've provide a bunch of general direction (maybe opinion, depending on your perspective) on this sort of thing in the Elements wiki. Would be great if we could have some discussion on that content in an upcoming DS working group session.

adrian-reapit commented 1 week ago

I'm not sure AppBar is a good name for this component. A user would assume its the top bar but it contains full site navigation, some of which is outside the 'bar'. We could call it ElementsNavigation.

The logo and search items should be fully customisable and positioned in placeholders but i think we should keep a consistent experience for the main navigation behaviour or it goes too far from the point of providing this library. The Logo component could allow full control of how the link works. @jmatiushina

<ElementsNavigation renderLogo={<MyLogo />}" renderSearch={<MySearchComponent />} mainNavigation={[]} secondaryNavigation={[]} appSwitcherNavigation={[]}>

We are looking to allow integration of React Router Link components, or similar from other router libraries, into the main navigation so you would have the same object style control of the href as in React Router. We are working on this but it should need a top level provider which would define the link component to be used as base for a button which would appear with the Elements button styling

kurtdoherty commented 1 week ago

I'm not sure AppBar is a good name for this component.

I think AppBar feels suitable if the component doesn't take on more responsibility than it should (I expand on this below).

The logo and search items should be fully customisable and positioned in placeholders but i think we should keep a consistent experience for the main navigation behaviour or it goes too far from the point of providing this library.

I think we'll need to have a verbal conversation on this. I do not believe a config-style API is appropriate here for an atomic component; it should not be the responsibility of an app bar component to determine the how navigation items are rendered, nor the interface those navigation items have.

EDIT: What you're proposing here feels like a higher-level component than I think we should be focused on right now. A shared component like ElementsNavigation could be really valuable to teams, but it could also not be valuable enough to warrant the effort. In my experience with the Console component library, the wrong abstraction is more costly than no abstraction at all. ElementsNavigation feels like an abstraction that is too early to think about; we should provide the primitives, let our products engineering teams use them, get feedback on the pain points, then provide higher-level abstractions once we're equipped to make informed decisions about the abstractions Elements consumers would benefit from.

We are looking to allow integration of React Router Link components, or similar from other router libraries, into the main navigation so you would have the same object style control of the href as in React Router. We are working on this but it should need a top level provider which would define the link component to be used as base for a button which would appear with the Elements button styling.

For solutions like this, I think we need to go through an RFC or solution design process in order to provide opportunity for input from consumers of Elements. In my opinion, this kind of thing isn't something Elements should be directly concerned about; it should be sufficient to provide atomic Link and Button components that consumers can easily integrate with their routing library of choice.