Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Possibly inconsistent ordering of classes in FL.com #514

Open
korbinancell opened this issue Sep 22, 2021 · 5 comments
Open

[BUG] Possibly inconsistent ordering of classes in FL.com #514

korbinancell opened this issue Sep 22, 2021 · 5 comments
Labels
bug Something isn't working

Comments

@korbinancell
Copy link
Contributor

Describe the bug
On different environments and/ or versions of styled-ui we are getting different orders of classes on styled components.

image

To Reproduce
Steps to reproduce the behavior:
Have a styled Tab.Panel where you set display: flex

export const EventPanel = styled(Tab.Panel).attrs({
	display: 'flex',
	flexDirection: 'column',
	alignItems: 'center',
	paddingX: 6,
	paddingTop: 5,
	paddingBottom: 6,
})`
	width: 100%;
`;

Have it work on sui v6.7.0 and not work on v6.8.1

@korbinancell korbinancell added the bug Something isn't working label Sep 22, 2021
@PatrickNausha
Copy link
Contributor

PatrickNausha commented Sep 23, 2021

Another place this problem can be reproduced is on a Faithlife.com page that renders @faithlife/asset-picker: https://git.faithlife.dev/Logos/FaithlifeEquipment/blob/d0ef9ef2440fce6aefdd5ba86f76841ac5664d41/packages/asset-picker/src/Tabs.tsx#L19

Note display="grid" on Tab.Panel below.

import React, { useEffect, useState } from 'react';
import { Tab } from '@faithlife/styled-ui';
import styled from 'styled-components';

export const Tabs = ({ children, selectedTab }) => (
	<Tab.Manager selectedTab={selectedTab}>
		<Tab.List>
			{React.Children.map(children, (tab) =>
				tab ? (
					<Tab paddingX={4}>
						<TabLabel title={tab.props.title} shortTitle={tab.props.shortTitle} />
					</Tab>
				) : null
			)}
		</Tab.List>
		<Tab.Panels display="grid" flexGrow={1}>
			{React.Children.map(children, (tab) =>
				tab ? (
					<Tab.Panel display="grid" padding={tab.props.padding || 0}>
						{({ selected }) => (
							<RenderChildrenIfWasEverSelected selected={selected}>
								{React.cloneElement(tab)}
							</RenderChildrenIfWasEverSelected>
						)}
					</Tab.Panel>
				) : null
			)}
		</Tab.Panels>
	</Tab.Manager>
);

/* ... */

I think we could fix styled-ui's Tab components by not relying on the ordering of CSS style declaration blocks. I think that'd mean not using a mix of styled components and styled-system objects like Box.

In the case of @faithlife/asset-picker, the two out-of-order style declarations are created by

  1. The display="grid" property applied to the Box component underlying Tab.Panel.
  2. The styled components display value.

The problem with EventPanel appears nearly identical except with Tab.Panel wrapped in a styled component.

@PatrickNausha
Copy link
Contributor

I've not been able to reproduce this problem outside of Faithlife.com. Even after copy-pasting the problematic code directly into a new application: https://git.faithlife.dev/PatrickNausha/styled-ui-weirdness-investigation/blob/master/src/App.js

@PatrickNausha
Copy link
Contributor

It appears that downgrading styled-components from 5.3.0 to 5.1.1 fixes this problem on Faithlife.com. styled-components/styled-components#3482 is the only issue I've found where others are having similar problems. Those problems are supposedly fixed in 5.3.1, but the issue still persists on Faithlife.com when using 5.3.1.

I've tried varying combinations of the following

  • styled-components versions
  • enabling/disabling SSR on Faithlife.com
  • removing/re-adding loadable-components

But the only thing that seems to work with styled-ui 6.8.1 is fixing styled-components to 5.1.1.

@PatrickNausha
Copy link
Contributor

I saw styled-components/styled-components#3482 (comment) and tried cramming @faithlife/styled-ui through babel-plugin-styled-components in Faithlife's build. I did so by tweaking Faithlife's exclude regex to allow @faithlife/styled-ui to be loaded by babel.

I ran yarn watch inside the Faithlife repo, let the build finish after many of the message below, refreshed the browser, but still saw the broken tab UI from styled-ui.

[BABEL] Note: The code generator has deoptimised the styling of /Users/patrick.nausha/code/Faithlife/node_modules/@faithlife/asset-picker-host/node_modules/@faithlife/styled-ui/dist/main.js as it exceeds the max of 500KB.

@PatrickNausha
Copy link
Contributor

Here's a screenshot for completeness. I just realized someone reading this issue may not understand exactly which UI bug I've been referring to. 😁

Expected

Screen Shot 2021-09-24 at 4 54 40 PM

Actual

Screen Shot 2021-09-24 at 4 53 34 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

2 participants