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.82k stars 1.17k forks source link

[Sticky] - Issue with initial rendering (using SSR) and sticky position #953

Closed loic-d closed 5 years ago

loic-d commented 5 years ago

Issue summary

The Sticky component seems to be impacting the initial rendering of the Automatic Discount page. The right column of the page is not rendered until you scroll. Then the sticky position is a bit off. I can't reproduce in production but I can on a local branch and master. Removing the Sticky component fixed the issue.

feb-01-2019 15-42-41

Expected behavior

The right column should be properly rendered and the sticky position should not be off.

Actual behavior

The right column is not properly rendered until you scroll in the page and the sticky position is off.

screen shot 2019-02-01 at 3 17 33 pm

Steps to reproduce the problem

  1. Load the React Automatic Discount page on your local env.

Reduced test case

The best way to get your bug fixed is to provide a reduced test case. This CodeSandbox template is a great starting point.

Specifications

loic-d commented 5 years ago

Wondering if it's related to SSR in web?

dleroux commented 5 years ago

At first I was thinking this was related to the latest frame changes but I just tested this playground and it's fine:

``` import * as React from 'react'; import { Page, AppProvider, Toast, Navigation, ContextualSaveBar, TopBar, Card, ActionList, Loading, Layout, SkeletonBodyText, SkeletonDisplayText, SkeletonPage, FormLayout, TextField, TextContainer, Stack, Frame, ResourceList, FilterType, Sticky, Modal, Button, } from '@shopify/polaris'; import {autobind} from '@shopify/javascript-utilities/decorators'; import { profile, logOut, home, orders, products, onlineStore, circlePlusOutline, } from '../src/icons'; interface State { showToast: boolean; isLoading: boolean; isDirty: boolean; searchActive: boolean; searchText: string; userMenuOpen: boolean; showMobileNavigation: boolean; modalActive: boolean; nameFieldValue: string; emailFieldValue: string; storeName: string; searchValue?: string; filterSearchFocused: boolean; selectedItems: string[] | 'All'; sortValue?: string; openModal: boolean; } export default class Playground extends React.Component { defaultState = { emailFieldValue: 'ellen@ochoacrafts.com', nameFieldValue: 'Ochoa Crafts', }; state: State = { showToast: false, isLoading: false, isDirty: false, searchActive: false, searchText: '', userMenuOpen: false, showMobileNavigation: false, modalActive: false, nameFieldValue: this.defaultState.nameFieldValue, emailFieldValue: this.defaultState.emailFieldValue, storeName: this.defaultState.nameFieldValue, selectedItems: [], filterSearchFocused: false, openModal: false, }; render() { const { showToast, isLoading, isDirty, searchActive, searchText, userMenuOpen, showMobileNavigation, nameFieldValue, emailFieldValue, modalActive, storeName, searchValue, openModal, } = this.state; const resourceName = { singular: 'Product', plural: 'Products', }; const toastMarkup = showToast ? ( ) : null; const navigationUserMenuMarkup = ( ); const contextualSaveBarMarkup = isDirty ? ( ) : null; const userMenuMarkup = ( ); const searchResultsMarkup = ( ); const searchFieldMarkup = ( ); const topBarMarkup = ( ); const navigationMarkup = ( ); const loadingMarkup = isLoading ? : null; const actualPageMarkup = ( } selectedItems={this.state.selectedItems} onSelectionChange={this.handleSelectionChange} promotedBulkActions={[ { content: 'Really long text on button 1', onAction: this.bulkActionOne, }, { content: 'long text button 2', disabled: true, url: 'http://www.google.com', }, ]} bulkActions={[ { content: 'button 3', onAction: this.bulkActionThree, }, { content: 'button 4', onAction: this.bulkActionFour, }, { content: 'button 5', onAction: this.bulkActionFive, disabled: true, }, ]} sortValue={this.state.sortValue} sortOptions={mockSortOptions} onSortChange={this.handleSortChange} /> this.setState({openModal: false})} > } onSelectionChange={this.handleSelectionChange} sortValue={this.state.sortValue} sortOptions={mockSortOptions} onSortChange={this.handleSortChange} />

Add tags to your order.

); const loadingPageMarkup = ( ); const pageMarkup = isLoading ? loadingPageMarkup : actualPageMarkup; const modalMarkup = ( Instagram logo

Sell your products directly on Instagram by tagging products in your posts, to create a seamless shopping experience for your customers.

); const theme = { colors: { topBar: { background: '#108043', }, }, logo: { width: 104, topBarSource: 'https://cdn.shopify.com/shopify-marketing_assets/static/shopify-full-color-white.svg', contextualSaveBarSource: 'https://cdn.shopify.com/shopify-marketing_assets/static/shopify-full-color-black.svg', }, }; return ( {contextualSaveBarMarkup} {loadingMarkup} {pageMarkup} {toastMarkup} {modalMarkup} ); } @autobind private handleSelectionChange(selectedItems: string[]) { this.setState({selectedItems}); } @autobind private handleSearchChange(searchValue: string) { this.setState({searchValue}); } @autobind private handleSortChange(sortValue: string) { this.setState({sortValue}); } @autobind private bulkActionOne() { console.log('Clicked on bulk action one.'); } @autobind private bulkActionThree() { console.log('Clicked on bulk action three.'); } @autobind private bulkActionFour() { console.log('Clicked on bulk action four.'); } @autobind private bulkActionFive() { console.log('Clicked on bulk action five.'); } toggleState = (key: string) => { return () => { this.setState((prevState) => ({[key]: !prevState[key]})); }; }; handleSearchFieldChange = (value: string) => { this.setState({searchText: value}); if (value.length > 0) { this.setState({searchActive: true}); } else { this.setState({searchActive: false}); } }; handleSearchResultsDismiss = () => { this.setState(() => { return { searchActive: false, searchText: '', }; }); }; handleEmailFieldChange = (emailFieldValue: string) => { this.setState({emailFieldValue}); if (emailFieldValue != '') { this.setState({isDirty: true}); } }; handleNameFieldChange = (nameFieldValue: string) => { this.setState({nameFieldValue}); if (nameFieldValue != '') { this.setState({isDirty: true}); } }; handleSave = () => { this.defaultState.nameFieldValue = this.state.nameFieldValue; this.defaultState.emailFieldValue = this.state.emailFieldValue; this.setState({ isDirty: false, showToast: true, storeName: this.defaultState.nameFieldValue, }); }; handleDiscard = () => { this.setState({ emailFieldValue: this.defaultState.emailFieldValue, nameFieldValue: this.defaultState.nameFieldValue, isDirty: false, }); }; } function handleRenderItem(item: any, id: any) { return (
Item {id}
{item.title}
); } const items: any[] = [ { onClick: true, url: 'https://www.google.com', actions: [{content: 'View listing', url: 'http://www.facebook.com'}], title: 'Has url, onClick, and actions', }, { onClick: false, url: 'https://www.google.com', actions: [{content: 'View listing', url: 'http://www.facebook.com'}], title: 'Has url, and actions', }, { onClick: true, actions: [{content: 'View listing', url: 'http://www.facebook.com'}], title: 'Has onClick, and actions', }, { onClick: false, url: 'https://www.google.com', title: 'Has url', }, { onClick: true, title: 'Has onClick', }, { onClick: true, url: 'https://www.google.com', actions: [{content: 'View listing', url: 'http://www.facebook.com'}], title: 'Has url, onClick, and actions', }, { onClick: false, url: 'https://www.google.com', actions: [{content: 'View listing', url: 'http://www.facebook.com'}], title: 'Has url, and actions', }, { onClick: true, actions: [{content: 'View listing', url: 'http://www.facebook.com'}], title: 'Has onClick, and actions', }, { onClick: false, url: 'https://www.google.com', title: 'Has url', }, { onClick: true, title: 'Has onClick', }, { onClick: true, url: 'https://www.google.com', actions: [{content: 'View listing', url: 'http://www.facebook.com'}], title: 'Has url, onClick, and actions', }, { onClick: false, url: 'https://www.google.com', actions: [{content: 'View listing', url: 'http://www.facebook.com'}], title: 'Has url, and actions', }, { onClick: true, actions: [{content: 'View listing', url: 'http://www.facebook.com'}], title: 'Has onClick, and actions', }, { onClick: false, url: 'https://www.google.com', title: 'Has url', }, { onClick: true, title: 'Has onClick', }, ]; const mockFilters: any[] = [ { key: 'filterKey1', label: 'Product type', operatorText: 'is', type: FilterType.Select, options: [ 'Bundle', { value: 'electronic_value', label: 'Electronic', disabled: true, }, { value: 'beauty_value', label: 'Beauty', }, ], }, { key: 'filterKey2', label: 'Tagged with', type: FilterType.TextField, }, ]; const mockSortOptions = [ 'Product title (A-Z)', { value: 'PRODUCT_TITLE_DESC', label: 'Product title (Z-A)', }, { value: 'EXTRA', label: 'Disabled Option', disabled: true, }, ]; ```
loic-d commented 5 years ago

Removing SSR seems to have fixed the issue. Wondering why we are only getting the error now?

dleroux commented 5 years ago

So we'll need to prevent rendering of Sticky on the server.

loic-d commented 5 years ago

The offset is still there even when disabling SSR

screen shot 2019-02-01 at 4 02 39 pm
dleroux commented 5 years ago

🤔 This also works in the playground

maxariss commented 5 years ago

Also affected by this 😢

Specifically, I'm affected by the bug where the position is wrong, and my sticky container is being overlapped by the TopBar.

Disabling SSR doesn't help either.

This is preventing the new <CollectionProductsSendTo /> from shipping:

gif

loic-d commented 5 years ago

@dleroux @maxariss So we have two issues:

dleroux commented 5 years ago

I'm not convinced that ssr is the culprit because when we start scrolling the calculations should fix themselves with the exception of the top bar. The tobBarOffset is calculated on ComponentDidMount of the app provider so this should happen on the client only. In Quilt the react-html package adds display: none to the body, so when we query the top bar clientHeight on mount clientHeight will return 0. Not sure why this wasn't an issue before, maybe it was always in prod but we never saw it because internal routes weren't enabled.

In any case, switching display: none to visibility: hidden seems to fix the issue. Not sure what the repercussions would be but this also works to hide the css flicker. @lemonmade I see you recently touched this code. Do you see potential issues form switching display to visibility?

dominiquesr commented 5 years ago

@dleroux @lemonmade Are either of you working on this issue? It's blocking the release of a feature we had planned to bug hunt two days ago

lemonmade commented 5 years ago

@dleroux I don't see a problem with it, no. If that is the source of this issue, it only affects dev, and might not actually be fixed in dev anyways because of how we inject the styles into the head after the JS loads (I am not positive that it will still break, but in dev it certainly corrects when you do anything that forces the sticky manager to recompute the position). Running the prod build will tell you for sure whether this is a development-only issue or not.

loic-d commented 5 years ago

From what I see, production does not seem to be impacted.

I could reproduce the issue on a local branch and I could not reproduce anymore in production once it 🚢 'ed

dleroux commented 5 years ago

Other than investigating i have not done additional work. It will need a change in Quilt. I will see if i can make time soon but if someone else has time go for it. @loic-d?

loic-d commented 5 years ago

@dleroux Are you suggesting not to render the component on the server?

dleroux commented 5 years ago

No. The react-html package in ‘Quilt’ sets the body style to ‘display: none’ while the page loads. We need to change that to ‘visibility: hidden’. Then we need a version bump and update web.

That will solve the top bar height to be registered in the ‘Sticky Manager’. I can’t reproduce your second issue but I have a feeling it’s related as well.

dleroux commented 5 years ago

PR created for this: https://github.com/Shopify/quilt/pull/515

ghost commented 5 years ago

This issue has been inactive for 180 days and labeled with Icebox. It will be closed in 7 days if there is no further activity.