Shopify / polaris

Shopify’s design system to help us work together to build a great experience for all of our merchants.
https://polaris.shopify.com
Other
5.81k stars 1.17k forks source link

Tab panel rendering incorrect when change tab inside an embedded app #176

Closed minhle2994 closed 7 years ago

minhle2994 commented 7 years ago

Issue summary

Tab rendering incorrect when change tab inside an embedded app.

Expected behavior

Tab list must be show after app header

Actual behavior

Part of tab list is under cover of app header.

Steps to Reproduce the Problem

  1. Embedded app with two tab like
class App extends Component {
    constructor(props) {
        super(props);
        this.state = {
            selectedTab: 0
        };
    }

    render() {
        return (
        < EmbeddedApp
            apiKey={apiKey}
            shopOrigin={shopOrigin}
            forceRedirect
        >

            < Page
            >
                <Tabs
                    selected={this.state.selectedTab}
                    onSelect={selectedTabIndex => this.setState({selectedTab: selectedTabIndex})}
                    tabs={[
                        {
                            id: 'tab1',
                            title: 'tab1',
                            panelID: 'tab1-content',
                        },
                        {
                            id: 'tab2',
                            title: 'tab2',
                            panelID: 'tab2-content',
                        },
                    ]}
                >

                    {(function(selectedTab) {
                        switch (selectedTab) {
                            case 0:
                                return (
                                    <Layout>
                                        <Layout.Section>
                                            <Banner
                                                title="Before you can purchase a shipping label, this change needs to be made:"
                                                action={{content: 'Edit address'}}
                                                status="warning"
                                            >
                                                <List>
                                                    <List.Item>The name of the city you’re shipping to has characters that aren’t allowed.</List.Item>
                                                </List>
                                            </Banner>
                                        </Layout.Section>
                                        <Layout.Section>
                                            <Banner
                                                title="Before you can purchase a shipping label, this change needs to be made:"
                                                action={{content: 'Edit address'}}
                                                status="warning"
                                            >
                                                <List>
                                                    <List.Item>The name of the city you’re shipping to has characters that aren’t allowed.</List.Item>
                                                </List>
                                            </Banner>
                                        </Layout.Section>
                                        <Layout.Section>
                                            <Banner
                                                title="Before you can purchase a shipping label, this change needs to be made:"
                                                action={{content: 'Edit address'}}
                                                status="warning"
                                            >
                                                <List>
                                                    <List.Item>The name of the city you’re shipping to has characters that aren’t allowed.</List.Item>
                                                </List>
                                            </Banner>
                                        </Layout.Section>
                                        <Layout.Section>
                                            <Banner
                                                title="Before you can purchase a shipping label, this change needs to be made:"
                                                action={{content: 'Edit address'}}
                                                status="warning"
                                            >
                                                <List>
                                                    <List.Item>The name of the city you’re shipping to has characters that aren’t allowed.</List.Item>
                                                </List>
                                            </Banner>
                                        </Layout.Section>
                                    </Layout>

                                );
                            case 1:
                                return (
                                    <Layout>
                                        <Layout.Section>
                                            <Banner
                                                title="Before you can purchase a shipping label, this change needs to be made:"
                                                action={{content: 'Edit address'}}
                                                status="warning"
                                            >
                                                <List>
                                                    <List.Item>The name of the city you’re shipping to has characters that aren’t allowed.</List.Item>
                                                </List>
                                            </Banner>
                                        </Layout.Section>
                                        <Layout.Section>
                                            <Banner
                                                title="Before you can purchase a shipping label, this change needs to be made:"
                                                action={{content: 'Edit address'}}
                                                status="warning"
                                            >
                                                <List>
                                                    <List.Item>The name of the city you’re shipping to has characters that aren’t allowed.</List.Item>
                                                </List>
                                            </Banner>
                                        </Layout.Section>
                                        <Layout.Section>
                                            <Banner
                                                title="Before you can purchase a shipping label, this change needs to be made:"
                                                action={{content: 'Edit address'}}
                                                status="warning"
                                            >
                                                <List>
                                                    <List.Item>The name of the city you’re shipping to has characters that aren’t allowed.</List.Item>
                                                </List>
                                            </Banner>
                                        </Layout.Section>
                                        <Layout.Section>
                                            <Banner
                                                title="Before you can purchase a shipping label, this change needs to be made:"
                                                action={{content: 'Edit address'}}
                                                status="warning"
                                            >
                                                <List>
                                                    <List.Item>The name of the city you’re shipping to has characters that aren’t allowed.</List.Item>
                                                </List>
                                            </Banner>
                                        </Layout.Section>
                                    </Layout>
                                );
                            default:

                        }
                    })(this.state.selectedTab)}
                </Tabs>
            </Page>
        </EmbeddedApp>
        );
    }
}

export default App;

Two tab need to be long enough.

  1. Switch between tabs

Specifications

dfmcphee commented 7 years ago

Thanks for filing the issue @minhle2994. Can you post a screenshot of the issue you are seeing here?

minhle2994 commented 7 years ago

Here is screenshots of the issue @dfmcphee Before change tab before

After change tab after

cgenevier commented 7 years ago

I have the same problem. It seems to happen intermittently, though, not every time I click a tab. I haven't been able to figure out what causes it.

Aditya94A commented 7 years ago

Any updates on this?

@minhle2994 @cgenevier Did you find any workarounds? I can't put this in production. This pretty much makes the tabs unusable :/

minhle2994 commented 7 years ago

@dfmcphee do you have any plan to fix this issue?

dfmcphee commented 7 years ago

I have been taking a look at this issue today. When clicking the tab it will focus the associated pane and scroll it into view. So that is expected behaviour.

There seems to be an issue with having the tab panes inside the tab element.

Can you try updating your example to something like this and see if you have the same problem?

class App extends Component {
    constructor(props) {
        super(props);
        this.state = {
            selectedTab: 0
        };
    }

    render() {
        return (
        < EmbeddedApp
            apiKey={apiKey}
            shopOrigin={shopOrigin}
            forceRedirect
        >

            < Page
            >
                <Tabs
                    selected={this.state.selectedTab}
                    onSelect={selectedTabIndex => this.setState({selectedTab: selectedTabIndex})}
                    tabs={[
                        {
                            id: 'tab1',
                            title: 'tab1',
                            panelID: 'tab1-content',
                        },
                        {
                            id: 'tab2',
                            title: 'tab2',
                            panelID: 'tab2-content',
                        },
                    ]}
                />
                    {(function(selectedTab) {
                        switch (selectedTab) {
                            case 0:
                                return (
                                    <Layout>
                                        <Layout.Section>
                                            <Banner
                                                title="Before you can purchase a shipping label, this change needs to be made:"
                                                action={{content: 'Edit address'}}
                                                status="warning"
                                            >
                                                <List>
                                                    <List.Item>The name of the city you’re shipping to has characters that aren’t allowed.</List.Item>
                                                </List>
                                            </Banner>
                                        </Layout.Section>
                                        <Layout.Section>
                                            <Banner
                                                title="Before you can purchase a shipping label, this change needs to be made:"
                                                action={{content: 'Edit address'}}
                                                status="warning"
                                            >
                                                <List>
                                                    <List.Item>The name of the city you’re shipping to has characters that aren’t allowed.</List.Item>
                                                </List>
                                            </Banner>
                                        </Layout.Section>
                                        <Layout.Section>
                                            <Banner
                                                title="Before you can purchase a shipping label, this change needs to be made:"
                                                action={{content: 'Edit address'}}
                                                status="warning"
                                            >
                                                <List>
                                                    <List.Item>The name of the city you’re shipping to has characters that aren’t allowed.</List.Item>
                                                </List>
                                            </Banner>
                                        </Layout.Section>
                                        <Layout.Section>
                                            <Banner
                                                title="Before you can purchase a shipping label, this change needs to be made:"
                                                action={{content: 'Edit address'}}
                                                status="warning"
                                            >
                                                <List>
                                                    <List.Item>The name of the city you’re shipping to has characters that aren’t allowed.</List.Item>
                                                </List>
                                            </Banner>
                                        </Layout.Section>
                                    </Layout>

                                );
                            case 1:
                                return (
                                    <Layout>
                                        <Layout.Section>
                                            <Banner
                                                title="Before you can purchase a shipping label, this change needs to be made:"
                                                action={{content: 'Edit address'}}
                                                status="warning"
                                            >
                                                <List>
                                                    <List.Item>The name of the city you’re shipping to has characters that aren’t allowed.</List.Item>
                                                </List>
                                            </Banner>
                                        </Layout.Section>
                                        <Layout.Section>
                                            <Banner
                                                title="Before you can purchase a shipping label, this change needs to be made:"
                                                action={{content: 'Edit address'}}
                                                status="warning"
                                            >
                                                <List>
                                                    <List.Item>The name of the city you’re shipping to has characters that aren’t allowed.</List.Item>
                                                </List>
                                            </Banner>
                                        </Layout.Section>
                                        <Layout.Section>
                                            <Banner
                                                title="Before you can purchase a shipping label, this change needs to be made:"
                                                action={{content: 'Edit address'}}
                                                status="warning"
                                            >
                                                <List>
                                                    <List.Item>The name of the city you’re shipping to has characters that aren’t allowed.</List.Item>
                                                </List>
                                            </Banner>
                                        </Layout.Section>
                                        <Layout.Section>
                                            <Banner
                                                title="Before you can purchase a shipping label, this change needs to be made:"
                                                action={{content: 'Edit address'}}
                                                status="warning"
                                            >
                                                <List>
                                                    <List.Item>The name of the city you’re shipping to has characters that aren’t allowed.</List.Item>
                                                </List>
                                            </Banner>
                                        </Layout.Section>
                                    </Layout>
                                );
                            default:

                        }
                    })(this.state.selectedTab)}
            </Page>
        </EmbeddedApp>
        );
    }
}

export default App;
cgenevier commented 7 years ago

Not sure if it helps, but I can confirm that in my case where the bug is happening I also have nested tabs (tabs inside of a tab panel).

dfmcphee commented 7 years ago

@cgenevier can you try moving the panels outside of the Tabs component and see if it resolves your issue?

<Tabs
  ...
/>
<Tabs.Panel>...</Tabs.Panel>
cgenevier commented 7 years ago

@dfmcphee I don't have Panels - my code is similar to OP's example.

minhle2994 commented 7 years ago

@dfmcphee many thanks, it fixed the issue now. I think you will have a clear example or document for tab layout.

dfmcphee commented 7 years ago

Ok great. I am going to close this issue since we already have one to improve the Tabs documentation in #62.

Aditya94A commented 7 years ago

:/

The issue is still very much present for me. I already had my Tabs.Panel outside of the Tabs element and when switching tabs the tabs scroll not only out of view but out of the iframe's viewport, that is I can't scroll back up to them, all I can see is the highlighted underline of the tabs. Plus the panel sometimes doesn't fill up the entire height of the container (after switching tabs for the first time)

@dfmcphee In your code snippet above, I don't see any Tabs.Panels at all, are we not supposed to use them?

cgenevier commented 7 years ago

@dfmcphee This is still an issue for me too, and my code is similar to the code you provided in this comment above.

dfmcphee commented 7 years ago

Ah, ok. I will reopen this. Can you shared a code example of what you are using @AdityaAnand1 @cgenevier? Will try to dig into this again.

cgenevier commented 7 years ago

@dfmcphee

I can't post exact code since there's a lot on the page and it's using different components for each different tab, but here's an example of how I have things structured.

Top-level component contains this code:

<Page
  title="Tab Example
  fullWidth
>
<Tabs
  selected={this.state.tab}
  tabs={[
    {
      id: 'overview',
      title: 'Overview',
      panelID: 'overview-content',
    },
    {
      id: 'activity',
      title: 'Activity',
      panelID: 'activity-content',
    }
  ]}
  onSelect={(val) => { this.setState({tab: val}); }}
>
  <div>
    {(() => {
      switch (this.state.tab) {
        case 0: return <OverviewComponent tab="0" />;
        case 1: return <ActivityComponent tab="1" />;
      }
    })()}
  </div>
</Tabs>
</Page>

And then for example, the ActivityComponent contains something that looks like:

<Layout>
<Layout.AnnotatedSection
  title="Activity Page"
  description="Description here"
>
<Card>
<Card.Section>
<Tabs
    selected={this.state.tab}
    tabs={[
        {
            id: 'tab1',
            title: 'Tab 1',
            panelID: 'tab1-content',
        },
        {
            id: 'tab2',
            title: 'Tab 2',
            panelID: 'tab2-content',
        }
    ]}
    onSelect={(val) => {
        this.setState({tab: val});
    }}
>
    <div style={{'marginTop':'2rem'}}>
        {(() => {
            switch (this.state.tab) {
                case 0: return <div>tab1</div>;
                case 1: return <div>tab2</div>;

            }
        })()}
    </div>
</Tabs>
</Card.Section>
</Card>
</Layout.AnnotatedSection>
</Layout>

Again, this is simplified since I have a lot more complexity in the app, but the structure is the same. In my top level component, I have 7 tabs, 2 of which call components which themselves contain tabs (such as in the above example). It's only when one of the 2 tabs themselves containing tabs is clicked that the issue occurs for me.

Aditya94A commented 7 years ago

@dfmcphee Here's a short example I just whipped up to showcase the problem:

Inside my render function for MyComponent, I've got two tabs with just dummy text inside them:

render() {
var tab1 = (
      <Tabs.Panel id={"tab1-content"}>
       <p>
Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed pellentesque sem in tortor pulvinar vulputate. Donec eu euismod nunc. Nulla ultrices congue commodo. Duis vel euismod lorem. Nunc non nulla magna. Vestibulum ante ipsum primis in faucibus orci luctus et ultrices posuere cubilia Curae; Pellentesque tincidunt diam nisi, et congue urna malesuada quis. Nulla aliquam neque nec blandit molestie. In hac habitasse platea dictumst. Sed nec aliquam sapien. Vestibulum ante ipsum primis in faucibus orci luctus et ultrices posuere cubilia Curae; Ut vel vestibulum lacus. Orci varius natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Quisque euismod ante erat, id maximus ex rutrum in. Pellentesque rutrum vel massa eget hendrerit.
<br/><br/>
Nulla porttitor enim sit amet nibh faucibus, non auctor lectus dapibus. Nunc hendrerit pulvinar ultricies. Pellentesque et elementum ante, id varius orci. Curabitur non faucibus lacus. Aenean ut elit eget massa cursus commodo efficitur id velit. Duis tincidunt ipsum odio, et porta ligula tincidunt et. Nunc tincidunt, arcu ac aliquet euismod, enim elit sagittis augue, ac lacinia nisl orci id diam. Pellentesque bibendum tempus blandit. Aenean luctus mauris non ex rutrum blandit. Ut imperdiet quam in aliquam elementum.
<br/><br/>
Lorem ipsum dolor sit amet, consectetur adipiscing elit. Ut vitae dolor porttitor dui eleifend vestibulum. Duis vulputate, dolor at bibendum feugiat, massa metus cursus nisi, sed malesuada leo tortor eget ex. Nullam facilisis sodales pellentesque. Vivamus eros erat, volutpat ut nulla eget, blandit porttitor nisi. Nulla feugiat magna ac nisl feugiat pretium. Donec sodales, lorem id placerat tempor, ligula ligula blandit erat, nec sodales leo orci quis quam.
<br/><br/>
Etiam porta tortor justo, a fermentum massa placerat sed. In hac habitasse platea dictumst. Donec interdum turpis non neque elementum facilisis. Duis iaculis est vitae quam eleifend pretium. Integer accumsan, lorem a pharetra condimentum, leo orci pellentesque magna, sit amet vestibulum tortor odio ut tortor. Ut quis diam nulla. Etiam nec blandit tellus, quis accumsan quam. Phasellus ut consequat ante, nec faucibus arcu. Ut dapibus ipsum non faucibus vulputate.
</p> </Tabs.Panel>
    );
    var tab2 = (
      <Tabs.Panel id={"tab2-content"}>
       <p>
Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed pellentesque sem in tortor pulvinar vulputate. Donec eu euismod nunc. Nulla ultrices congue commodo. Duis vel euismod lorem. Nunc non nulla magna. Vestibulum ante ipsum primis in faucibus orci luctus et ultrices posuere cubilia Curae; Pellentesque tincidunt diam nisi, et congue urna malesuada quis. Nulla aliquam neque nec blandit molestie. In hac habitasse platea dictumst. Sed nec aliquam sapien. Vestibulum ante ipsum primis in faucibus orci luctus et ultrices posuere cubilia Curae; Ut vel vestibulum lacus. Orci varius natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Quisque euismod ante erat, id maximus ex rutrum in. Pellentesque rutrum vel massa eget hendrerit.
<br/><br/>
Nulla porttitor enim sit amet nibh faucibus, non auctor lectus dapibus. Nunc hendrerit pulvinar ultricies. Pellentesque et elementum ante, id varius orci. Curabitur non faucibus lacus. Aenean ut elit eget massa cursus commodo efficitur id velit. Duis tincidunt ipsum odio, et porta ligula tincidunt et. Nunc tincidunt, arcu ac aliquet euismod, enim elit sagittis augue, ac lacinia nisl orci id diam. Pellentesque bibendum tempus blandit. Aenean luctus mauris non ex rutrum blandit. Ut imperdiet quam in aliquam elementum.
<br/><br/>
Lorem ipsum dolor sit amet, consectetur adipiscing elit. Ut vitae dolor porttitor dui eleifend vestibulum. Duis vulputate, dolor at bibendum feugiat, massa metus cursus nisi, sed malesuada leo tortor eget ex. Nullam facilisis sodales pellentesque. Vivamus eros erat, volutpat ut nulla eget, blandit porttitor nisi. Nulla feugiat magna ac nisl feugiat pretium. Donec sodales, lorem id placerat tempor, ligula ligula blandit erat, nec sodales leo orci quis quam.
<br/><br/>
Etiam porta tortor justo, a fermentum massa placerat sed. In hac habitasse platea dictumst. Donec interdum turpis non neque elementum facilisis. Duis iaculis est vitae quam eleifend pretium. Integer accumsan, lorem a pharetra condimentum, leo orci pellentesque magna, sit amet vestibulum tortor odio ut tortor. Ut quis diam nulla. Etiam nec blandit tellus, quis accumsan quam. Phasellus ut consequat ante, nec faucibus arcu. Ut dapibus ipsum non faucibus vulputate.
</p>  </Tabs.Panel>
    );
    var showTab = this.state.selectedTab === 0 ? tab1 : tab2;

return (
      <Page>
        <Tabs
          selected={this.state.selectedTab}
          tabs={[
            {
              title: "tab1",
              id: "tab1",
              panelID: "tab1-content"
            },
            {
              title: "tab1",
              id: "tab1",
              panelID: "tab1-content"
            }
          ]}
          onSelect={index => {
            this.setState({ selectedTab: index });
          }}
        />
        <br />
        {showTab}
        <br />
      </Page>
    );
}

Also, I'm using react router dom, but I don't see how it could be messing with this.

My top level render function is this:

render() {
        return (
            <div>
                <Route
                    exact
                    path="/"
                    component={() => <MyComponent/>}
                />
            </div>
        );
    }

Steps to reproduce:

I'm on the latest chrome, windows, 1920p res. Let me know if you're unable to reproduce this. I'll make a video showing the problem.

chloerice commented 7 years ago

@dfmcphee So it appears that the problem isn't actually with the Tabs focus behaviour or the Tabs component at all. It's actually a bug within the admin itself in Chrome only, Firefox and Safari don't have this issue.

The admin consists of 3 sections:

For reasons that I have yet to nail down, when toggling into a tab whose panel content needs to be scrolled to be viewed in its entirety, the AppFrameMain moves upward by the equivalent of the height of the top bar. After playing around with it I've found that the best fix is to give the AppFrameMain a position of fixed (just like the AppFrameAside has), and changing the padding-left to margin-left so that the main doesn't overlap the aside (rendering the sidebar navigation unclickable).

With AppFrameMain not positioned:

appframemainnotfixed

With AppFrameMain having fixed position (and top/right/bottom/left at 0):

appframemainfixed

Here's the code I used for my embedded app: ```jsx import React, {Component} from 'react'; import { Page, Layout, List, Banner, Button, Tabs, } from '@shopify/polaris'; import {EmbeddedApp} from '@shopify/polaris/embedded'; class App extends Component { constructor(props) { super(props); this.state = { selectedTab: 0 }; } handleSelect = selectedTab => { this.setState({selectedTab}) } render() { const firstTabContent = ( The name of the city you’re shipping to has characters that aren’t allowed. The name of the city you’re shipping to has characters that aren’t allowed. The name of the city you’re shipping to has characters that aren’t allowed. The name of the city you’re shipping to has characters that aren’t allowed. ); const secondTabContent = ( The name of the city you’re shipping to has characters that aren’t allowed. The name of the city you’re shipping to has characters that aren’t allowed. The name of the city you’re shipping to has characters that aren’t allowed. The name of the city you’re shipping to has characters that aren’t allowed. ); const tabContent = this.state.selectedTab === 1 ? secondTabContent : firstTabContent; return ( {tabContent} ); } } export default App; ```
chloerice commented 7 years ago

Just an update on progress for fixing this issue: @tmlayton took the time to give me more context on the EASDK set up and also did a very thorough isolation of the bug. Essentially he found that it's happening because of the way that the embedded app iframe is sized and (though it took a unicorn's worth of digging) the fix is actually pretty simple and will be ready tomorrow.

cgenevier commented 7 years ago

This appears to still be a problem in 1.8.3. Any updates on when this will be fixed?

chloerice commented 7 years ago

Yes, you're correct @cgenevier, the fix has not yet been released. It turns out that the problem isn't within Polaris or with the Tabs themselves. It's a bug within some logic in the Shopify platform code that calculates the size of the iframe. We've got a fix ready, the pull request is currently under review and should ship by beginning of next week!

cgenevier commented 7 years ago

Any updates on this, @chloerice? I've had clients complaining about it for a while now.

chloerice commented 7 years ago

Hi @cgenevier, we're still actively working on the fix for this, as the solution we had when I last commented breaks in iOS.

chloerice commented 7 years ago

@cgenevier the fix is live 🎉 !

cgenevier commented 7 years ago

Awesome! Looks like it's working. Thank you so much!

bluedge commented 6 years ago

Hi, I'm using Polaris 1.9.1 (EASDK) and the scroll behavior described earlier is still present for me. Any update on a fix?

chloerice commented 6 years ago

@bluedge Are the tabs hidden under the topbar after toggling between tabs? Or which scroll behavior are you experiencing?

bluedge commented 6 years ago

Yes. the page below the top bar scrolls up. See a normal view with a short page:

screen shot 2018-01-17 at 10 40 40 am

After a click on the tab, with a longer than screen page (space before the tabs items is smaller):

screen shot 2018-01-17 at 10 40 55 am
chloerice commented 6 years ago

If you're in Chrome the scroll behavior is expected, as it focuses the first focusable item within the tab content, but you should still be able to scroll to the top of the page (revealing the rest of the padding above the tabs). Are you not able to scroll up (from the position in your second screenshot) after switching tabs?

bluedge commented 6 years ago

@chloerice I'm on chrome yes. I'm able to scroll up but that looks buggy. Maybe something in the same spirit as e.preventDefault(); after the tab is updated could solve this?

chloerice commented 6 years ago

@bluedge That is the expected behavior, Chrome focuses the first focusable item in your tab's content for screenreader accessibility.

capatina commented 5 years ago

@chloerice Hello, we are still having this issue on polaris 3.12.0

capatina commented 5 years ago

Tab with small content is fine:

Capture1

After clicking "Translation" the whole page scrolls down, making interaction very strange. Now I don't know where I am as a user: Capture2

capatina commented 5 years ago

The workaround were using:

componentDidUpdate(){ const page = document.querySelectorAll('html')[0]; page.scrollTop = 0; }

hoan2609 commented 2 years ago

I can't use Tabs.Panel. When I use Tabs.Panel Error I get when compiled: Property 'Panel' does not exist on type '(props: TabsProps) => Element'.ts(2339)

` const tabs = [ { id: "Security", title: "Security", panelID: "panel1", }, { id: "General", title: "General", panelID: "panel2", }, { id: "Contact", title: "Contact", panelID: "panel3", }, ];

const tabPanels = [

Description...

} {...fields.user_email} /> Description...

} {...fields.user_pass} /> display_name...

} {...fields.display_name} /> user_numberphone...

} {...fields.user_numberphone} />