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

fix: Add missing colour vars and Column export, lineClamp support, Color, Heading, Text demos #471

Merged
merged 13 commits into from
Mar 18, 2021

Conversation

frankieyan
Copy link
Member

@frankieyan frankieyan commented Mar 16, 2021

Short description

This adds a couple of improvements to the Design System components:

  • Exports the missing Column component
  • Adds padding prop to Text and Heading components
  • Adds lineClamp prop to Text and Heading components
  • Adds the largest value to the size prop for the Heading component
  • Adds the complete set of Doist colours, as a few colours used by the Text component were not defined
    • This doesn't include dark theme colours, as I couldn't think of a good way to have these defined in the library and consumed from client apps. We'll need to brainstorm how we can tackle this.
  • Adds Storybook demos for Heading. Text, and our available colours

We still have issues with stylesheets in the gh-pages build of storybook, but this would at least give us a reference in dev mode before we can fix it.

image

image

image

PR Checklist

  • Added tests for bugs / new features
  • Updated docs (storybooks, readme)
  • Executed npm run validate and made sure no errors / warnings were shown
  • Described changes in CHANGELOG.md
  • Bumped version in package.json and package-lock.json (npm --no-git-tag-version version <major|minor|patch>) ref
  • Updated all static build artifacts (npm run build-all)

Versioning

v9.1.0-beta.3

@frankieyan frankieyan requested review from a team, pedroalves0 and gnapse and removed request for a team March 16, 2021 13:47
@frankieyan frankieyan self-assigned this Mar 16, 2021
@gnapse
Copy link
Contributor

gnapse commented Mar 16, 2021

Adds padding prop to Text and Heading components

Do we really need this? There's a bit of an intended tendency to restrict what we can do with regards to customizing the design of some components. While Braid goes to the opposite end and disallows many of these things in most of their components, I made it so that the main layout components can do this without having to be wrapped inside an additional Box. However, I left these typography components out of it intentionally. The thinking is, they are not for controlling layout, they are for controlling typography. If they need spacing around them, it is not their concern.

Would love to hear your thoughts about this.

PS: Will review in full later.

@frankieyan frankieyan force-pushed the frankie/add-missing-exports-variables branch from aaad5b1 to 9d100d8 Compare March 16, 2021 13:56
<section className="story">
<Stack space="medium">
<Text size="xlarge" weight="strong" tone="secondary">
Text, xlarge, strong, secondary (16px)
Copy link
Member Author

Choose a reason for hiding this comment

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

Should xlarge use a larger font-size?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you ask? Is it using the same size as the previous one in the scale? If so, then maybe yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, both are 16px. I've created a post to align with design and you on this so we may make more changes here after 👍

@frankieyan
Copy link
Member Author

The thinking is, they are not for controlling layout, they are for controlling typography. If they need spacing around them, it is not their concern.

Yeah, I guess that makes sense. I can remove the padding addition tomorrow and use it a little while longer before seeing whether this is truly needed.

If you review before I do this, just ignore those changes - I'll drop the commit and update the storybook demo 👍

@frankieyan
Copy link
Member Author

If we have elements in a stack (or inline) with non-uniform spacing, then I guess the alternative for now is to use <Box> - either as a wrapper, or perhaps as <Box component={Heading} />. The latter might be better to avoid having an additional DOM node created.

@gnapse
Copy link
Contributor

gnapse commented Mar 16, 2021

If we have elements in a stack (or inline) with non-uniform spacing, then I guess the alternative for now is to use <Box> - either as a wrapper, or perhaps as <Box component={Heading} />. The latter might be better to avoid having an additional DOM node created.

That last one is indeed a very good idea!

I'll add a third option for a case like this: push back to the designer to see if we can get by with the uniform approach. It may not always go through, but in my experience with Alex during the new settings DO, it has worked a few times.

@frankieyan frankieyan changed the title fix: Add missing colour vars, Column component export, and spacing props to typography components fix: Add missing colour vars and Column component export; add Color, Heading, Text storybook demos Mar 17, 2021
@frankieyan frankieyan changed the title fix: Add missing colour vars and Column component export; add Color, Heading, Text storybook demos fix: Add missing colour vars and Column export; add Color, Heading, Text demos Mar 17, 2021
@frankieyan frankieyan changed the title fix: Add missing colour vars and Column export; add Color, Heading, Text demos fix: Add missing colour vars and Column export, lineClamp support, Color, Heading, Text demos Mar 17, 2021
h4.size-larger,
h5.size-larger,
h6.size-larger {
font-size: var(--reactist-font-size-large);
}

/* h4/h5/h6 can't be made smaller, maybe we reconsider this? */

/* truncated text */
Copy link
Contributor

@gnapse gnapse Mar 17, 2021

Choose a reason for hiding this comment

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

From the use of this feature in the stories, it seems this is to truncate to a certain number of lines, right? And when using lineClamp={1}, is it equivalent to Braid's truncate prop? (ref).

I ask because I recently introduced in Todoist the truncate prop and was meaning to bring it to Reactist these days. But this line clamp thing seems to be more comprehensive if it covers multi-line cases, while still behaving as truncate for a single line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah .line-clamp-1 applies text-overflow: ellipsis; instead of -webkit-line-clamp.

The vendor prefixes are required since autoprefixer doesn't handle this for now: postcss/autoprefixer#1322

src/new-components/heading/heading.tsx Outdated Show resolved Hide resolved
src/new-components/heading/heading.tsx Outdated Show resolved Hide resolved
Comment on lines +36 to +59
.line-clamp-1 {
text-overflow: ellipsis;
white-space: nowrap;
overflow: hidden;
}

.lineClamp {
display: -webkit-box;
-webkit-box-orient: vertical;
overflow: hidden;
}

.line-clamp-2 {
-webkit-line-clamp: 2;
}
.line-clamp-3 {
-webkit-line-clamp: 3;
}
.line-clamp-4 {
-webkit-line-clamp: 4;
}
.line-clamp-5 {
-webkit-line-clamp: 5;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If we ever need this in a third component we should consider extracting it into a reusable CSS module that any component module could import.

<section className="story">
<Stack space="medium">
<Text size="xlarge" weight="strong" tone="secondary">
Text, xlarge, strong, secondary (16px)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you ask? Is it using the same size as the previous one in the scale? If so, then maybe yes.

@frankieyan frankieyan force-pushed the frankie/add-missing-exports-variables branch from 84745ba to 0e5dcbe Compare March 18, 2021 07:25
@frankieyan frankieyan force-pushed the frankie/add-missing-exports-variables branch from 0e5dcbe to bfe860f Compare March 18, 2021 11:10
@frankieyan frankieyan force-pushed the frankie/add-missing-exports-variables branch from 008e568 to 95066bd Compare March 18, 2021 11:32
@frankieyan frankieyan merged commit f25a1dc into beta Mar 18, 2021
@frankieyan frankieyan deleted the frankie/add-missing-exports-variables branch March 18, 2021 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants