[Page]✨ add forceRender to force render of the title, actions & breadcrumbs to be in page vs in the top bar.

[michenly]

WHY are these changes introduced?

Resolves #669 (comment)

WHAT is this pull request doing?

Added forceRender to title, breadcrumbs, and actions render in the app when app-bridge is use

Default using app bridge
ie. <AppProvider apiKey={YOUR_API_KEY}>

Default without app bridge
ie. <AppProvider apiKey={undefined}>

Using app bridge & forceRenderTitle
ie. <AppProvider apiKey={YOUR_API_KEY}><Page title="test title" forceRenderTitle /></AppProvider>

Using app bridge & forceRenderActions
ie. <AppProvider apiKey={YOUR_API_KEY}><Page title="" primaryAction={{content: 'Save'}} forceRenderActions /></AppProvider>

Using app bridge, forceRenderTitle, and forceRenderActions
ie. <AppProvider apiKey={YOUR_API_KEY}><Page title="test title" primaryAction={{content: 'Save'}} forceRenderTitle forceRenderActions /></AppProvider>

Using app bridge, forceRender
ie. <AppProvider apiKey={YOUR_API_KEY}><Page title="test title" primaryAction={{content: 'Save'}} forceRender /></AppProvider>

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:

import * as React from 'react';
import {Page} from '@shopify/polaris';

interface State {}

export default class Playground extends React.Component<never, State> {
  render() {
    return (
      <AppProvider apiKey={YOUR_API_KEY}>
          <Page title="Playground" breadcrumbs={[{content: 'previous page'}]} primaryAction={{content: 'Save'}}>
            {/* Add the code you want to test here */}
          </Page>
      </AppProvider>
    );
  }
}

🎩 checklist

• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility
• Updated UNRELEASED.md

[resistorsoftware]

I am using latest Polaris 3.1.1 and my Page component is not respecting this idea of forceRenderingActions seriously.. my setup is..

<Page
    title="Dashboard"
    icon={this.props.inventory.icon}
    primaryAction={{content: 'Recommendations', target: 'APP', url: '/recommendations'}}
    forceRenderActions
  >

And yet my primaryAction button is rendering in that ugly separate top heading bar... what? I am now super confused. I am using AppProvider with an API key, so this is App Bridge enabled. Whatever that means. I am so disappointed at the level of confusion introduced by all these breaking changes it is disheartening to say the least.

[michenly]

@resistorsoftware the idea of forceRenderActions is only in this PR and and is meant to be a possible way of addressing exactly the confusion you mention above.

This is unfortunately not part of Polaris 3.1.1 yet so the only way to see forceRenderActions in action is pulling this branch locally.

There are a few work around mentioned in #669 to have this working today. (I personally like #669 (comment) the most)

I hope this help!

[resistorsoftware]

Thanks @michelleyschen

Personally, I hate wasting my time, and using workarounds is just that. I am not into re-visiting work a few months or longer down the road to simply remove workarounds that fix what should be fixed for real, now.

I am seeing countless complaints and errors and bugs with App Bridge et al... including what happened last night. The button I see (rendered in the "wrong" place belongs to a completely different App running in a different tab, in the same shop!

I am uncomfortable being a guinea pig as the Shopify team attempts to bring the App Bridge to life, and also, I am not pleased the documentation and directions do not follow when implementing Polaris. It should be WYSIWYG but instead, as this shows, it is more like chaos.

[amooz]

Quick note: whenever you're comparing the string '', always consider if you need to trim() the string first to avoid bugs using extra spaces at the end. I think this check will pass if title is " ", which doesn't look like the desired behaviour? Could be wrong, though! :)

[michenly]

Because this is the existing logic in Polaris, I am un-sure about changing the check @tmlayton double checking if this is the desired behaviour or can I introduce the trim check?

[resistorsoftware]

Do we have to wait till 2019 for Polaris to work with an Embedded App and Page component? Why is it taking so long to fix and make it consistent with the dreams and documentation?

[tmlayton]

@michelleyschen I’d prefer to keep this approach a bit simpler where forceRender skips delegation to App Bridge entirely. Then if an app still wants to use App Bridge’s TitleBar, they can pull the appBridge instance off of context and interact with app bridge directly.

Personally, I’m leaning heavily towards removing app bridge from Polaris entirely. It seems to have caused more issues and confusion than it has been helpful. I’m thinking we would still provide a set of React components which wrap app bridge (maybe even keep the same names), as well as an AppBridgeProvider for instantiation. Wherever these components live, we can move the ShopifyRoutePropagator there as well.

@resistorsoftware the criticisms are appreciated. Given I did most of the work integrating App Bridge with Polaris, it’s disappointing it hasn’t gone smoothly. It’s a fair point about being forced into early adoption (guinea pig) with App Bridge. While App Bridge is ultimately the path forward at Shopify, we were too optimistic including it in v3.0.0 as App Bridge was being launched.

That being said, this is a pull request. Please keep comments specific to @michelleyschen’s approach to force-rendering <Page /> when in an embedded context and direct issues and criticisms to either GitHub issues or, for App Bridge specifically, the Shopify APIs & SDKs forum.

[tmlayton]

I'm thinking only a forceRender prop which simply skips app bridge delegation.

[katbuilt]

@michelleyschen -- I chatted with @nwtn about this and I'm very supportive of this from a Merchant Experience of Apps perspective. I have two clarifying questions though:

1. The screenshots here only show the placement of primary action in relation to breadcrumb and title (purple button) Could you post some screenshots of what happens when there are primary actions and the maximum number of allowed secondary action buttons? What about apps using pagination?

2. What would be the backwards-facing implications of this PR to apps who don't add the forceRender to their app code?

Thank you!

[vividviolet]

I am seeing countless complaints and errors and bugs with App Bridge et al... including what happened last night.

@resistorsoftware can you clarify who is complaining? Is is the merchant or app developers?

The button I see (rendered in the "wrong" place belongs to a completely different App running in a different tab, in the same shop!

Can you please log an issue in this repo so we can dig into it?

Personally, I’m leaning heavily towards removing app bridge from Polaris entirely. It seems to have caused more issues and confusion than it has been helpful

I created a new issue so we can discuss removing App Bridge from Polaris #814

[michenly]

@katbuilt

The screenshots here only show the placement of primary action in relation to breadcrumb and title (purple button) Could you post some screenshots of what happens when there are primary actions and the maximum number of allowed secondary action buttons? What about apps using pagination?

The forceRender meant the app will render like any app without app bridge connection. So the actions buttons & pagination will work exactly like what the Page component listed on polaris guide. https://polaris.shopify.com/components/structure/page#navigation

What would be the backwards-facing implications of this PR to apps who don't add the forceRender to their app code?

Any app or Page that does not have forceRender will continue to work like today.

[michenly]

@tmlayton update the code to just have forceRender prop. The build-consumer seems to be broken, how have you been testing appBridge related things recently?

[tmlayton]

One comment about the doc link and looks like a bad rebase of the changelog, otherwise this looks fine.

[tmlayton]

Changelog is still including entries that have since been released

[keyfer]

Don't know how I missed that twice, sorry @tmlayton.

Should be good now.

[michenly]

I can't approve this since I am the original author lol

[michenly]

are we...updating the doc in this link?

Re-review.

[ghost]

🎉 Thanks for your contribution to Polaris React!