Skip to content

fix(styles): change all shorthand border styles to be longhand - #3172

Merged
gergelyke merged 6 commits into
uber:masterfrom
jasongi-actu:longhand-borders
Apr 9, 2020
Merged

fix(styles): change all shorthand border styles to be longhand#3172
gergelyke merged 6 commits into
uber:masterfrom
jasongi-actu:longhand-borders

Conversation

@jasongi-actu

@jasongi-actu jasongi-actu commented Apr 8, 2020

Copy link
Copy Markdown
Contributor

Fixes #3159

Description

I've run a few regexes over the codebase to remove border shorthand, and then manually fixed up/refactored some of ternary styles to not repeat logic.

There is on issue that the Border definition in the theme isn't longhand and is sometimes directly injected into components e.g here

...$theme.borders.border200,

This is going to cause some warning about mixing styles unless either the API changes or the border styles are transformed into longhand before being used.

Scope

  • Patch: Bug Fix
  • Minor: New Feature
  • Major: Breaking Change

@vercel

vercel Bot commented Apr 8, 2020

Copy link
Copy Markdown

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/uber-ui-platform/baseweb/2k7egaz8j
✅ Preview: https://baseweb-git-fork-jasongi-actu-longhand-borders.uber-ui-platform.now.sh

@codesandbox-ci

codesandbox-ci Bot commented Apr 8, 2020

Copy link
Copy Markdown

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 018b8c8:

Sandbox Source
immutable-dust-bb6k6 Configuration

@UberOpenSourceBot

Copy link
Copy Markdown
Contributor

Visual changes were detected on this branch. Please review the following PR containing updated snapshots: jasongi-actu#2

@chasestarr

Copy link
Copy Markdown
Collaborator

Thank you @jasongi-actu. Please review the visual regression test changes here: jasongi-actu#2 and ensure there are no changes. Those screenshots are determined by .scenario.js files.

@jasongi-actu

Copy link
Copy Markdown
Contributor Author

OK so the regressions were due to a couple of things. The aspect ratio one was just an error where width and style got mixed up by my regex.

The data table regression was a bit more fiddly though because the way the data table applied borders mixed both borderColor/borderWidth/borderStyle and borderLeft/borderRight/borderTop/borderBottom in a way that meant you could just drop in long hand styles and make it work. What complicates things more is that Theme doesn't use longhand styles, it uses borderColor/borderWidth/borderStyle. The way I see it there are two ways to fix this:

  1. API breaking change of the Theme object to have longhand styles instead of borderColor/borderWidth/borderStyle
  2. Find every instance of ...$theme.borders.borderXXX and replace it with the longhand styles (and provide a helper function to do that). This wouldn't break existing themes.

I've provided an implementation of 2, which wraps all the spreads of borders in a utility function, but it does add complexity and there is still the potential for user error when providing their own components/overrides to use longhand styles since that is what happens in the theme.

@gergelyke

Copy link
Copy Markdown
Contributor

Option 2 seems reasonable

@UberOpenSourceBot

Copy link
Copy Markdown
Contributor

Visual changes have been resolved. jasongi-actu#2 has been closed. If future commits on jasongi-actu:longhand-borders trigger visual changes, a new snapshot branch will be created and a new PR will be opened.

@nadiia nadiia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Styling] Longhand CSS attributes not used consistently for borders

5 participants