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

feat(Timeline): Migrate to styled-components #1468

Merged
merged 6 commits into from
Sep 25, 2020

Conversation

m7kvqbe1
Copy link
Collaborator

@m7kvqbe1 m7kvqbe1 commented Sep 22, 2020

Related issue

Closes #951

Overview

Initial migration away from SASS to styled-components.

Reason

SASS + BEM is brittle and makes it difficult for a consumer to override the presentation layer.

Work carried out

  • Migrate from SASS to styled-components
  • Remove broken box-shadow from sidebar
  • Use babel v7 config file format
  • Transpile @royalnavy/design-tokens when using Jest
  • Fix 'custom hours' story styling
  • Mark some items in roadmap as complete
  • Preserve BEM class names for internal skeleton markup

Developer notes

Design tokens package to be supplemented with helpers in proceeding PR:

https://www.erikverweij.dev/blog/manage-design-tokens-with-typescript-and-styled-components
https://github.com/everweij/design-tokens-ts-styled-components

@m7kvqbe1 m7kvqbe1 added Type: Enhancement New feature or request Package: react-component-library Package/code type Package: css-framework Package/code type Package: design-tokens Package/code type labels Sep 22, 2020
@m7kvqbe1 m7kvqbe1 self-assigned this Sep 22, 2020
@m7kvqbe1 m7kvqbe1 force-pushed the feat/timeline-styled-components branch from df42e75 to b8e82a4 Compare September 22, 2020 17:25
@m7kvqbe1 m7kvqbe1 force-pushed the feat/timeline-styled-components branch from cc36220 to ce26177 Compare September 23, 2020 10:19
@m7kvqbe1 m7kvqbe1 force-pushed the feat/timeline-styled-components branch from 5722092 to 408e279 Compare September 23, 2020 10:43
@m7kvqbe1 m7kvqbe1 force-pushed the feat/timeline-styled-components branch 2 times, most recently from 1a13746 to d9e7019 Compare September 23, 2020 11:09
@m7kvqbe1 m7kvqbe1 changed the title [WIP] feat(Timeline): Migrate to styled-components feat(Timeline): Migrate to styled-components Sep 23, 2020
@m7kvqbe1 m7kvqbe1 marked this pull request as ready for review September 23, 2020 11:11
@m7kvqbe1 m7kvqbe1 force-pushed the feat/timeline-styled-components branch from d9e7019 to d412895 Compare September 23, 2020 11:16
@m7kvqbe1 m7kvqbe1 force-pushed the feat/timeline-styled-components branch from db85f19 to a19c508 Compare September 23, 2020 17:23
packages/design-tokens/src/tokens.ts Outdated Show resolved Hide resolved
packages/design-tokens/src/tokens.ts Outdated Show resolved Hide resolved
packages/design-tokens/src/tokens.ts Outdated Show resolved Hide resolved
packages/design-tokens/src/tokens.ts Outdated Show resolved Hide resolved
packages/design-tokens/src/tokens.ts Outdated Show resolved Hide resolved
@m7kvqbe1 m7kvqbe1 force-pushed the feat/timeline-styled-components branch from a19c508 to d412895 Compare September 23, 2020 17:31
Copy link
Collaborator

@thyhjwb6 thyhjwb6 left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Some feedback around naming convention which looks like it was thought about but it feels useful to know that child elements of a styled component are styled components themselves.

@codecov
Copy link

codecov bot commented Sep 24, 2020

Codecov Report

Merging #1468 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1468      +/-   ##
==========================================
+ Coverage   98.37%   98.40%   +0.02%     
==========================================
  Files         167      167              
  Lines        1661     1692      +31     
  Branches      404      413       +9     
==========================================
+ Hits         1634     1665      +31     
  Misses         27       27              
Impacted Files Coverage Δ
...onent-library/src/components/Timeline/Timeline.tsx 100.00% <0.00%> (ø)
...onent-library/src/components/Timeline/constants.ts 100.00% <0.00%> (ø)
...nt-library/src/components/Timeline/TimelineDay.tsx 100.00% <0.00%> (ø)
...nt-library/src/components/Timeline/TimelineRow.tsx 100.00% <0.00%> (ø)
...t-library/src/components/Timeline/TimelineDays.tsx 100.00% <0.00%> (ø)
...t-library/src/components/Timeline/TimelineHour.tsx 100.00% <0.00%> (ø)
...t-library/src/components/Timeline/TimelineRows.tsx 100.00% <0.00%> (ø)
...t-library/src/components/Timeline/TimelineWeek.tsx 100.00% <0.00%> (ø)
...-library/src/components/Timeline/TimelineEvent.tsx 100.00% <0.00%> (ø)
...-library/src/components/Timeline/TimelineHours.tsx 100.00% <0.00%> (ø)
... and 5 more

@m7kvqbe1 m7kvqbe1 force-pushed the feat/timeline-styled-components branch from 87a0c1c to 4924956 Compare September 24, 2020 10:52
@jpveooys
Copy link
Collaborator

My only general question/comment is about the overriding of inner styles not covered by render props (e.g. SyledInner). Is the aim to use this pattern?

It would be good to have a story for that (not necessarily in this PR if there are further PRs coming).

@m7kvqbe1
Copy link
Collaborator Author

My only general question/comment is about the overriding of inner styles not covered by render props (e.g. SyledInner). Is the aim to use this pattern?

It would be good to have a story for that (not necessarily in this PR if there are further PRs coming).

We have literally just been discussing this - we're not quite sure yet. The idea of having a customClasses prop (map of custom classes to internal skeleton markup) came up.

I'm trying to get my head around how the example you linked would work for you the consumer.

Copy link
Collaborator

@thyhjwb6 thyhjwb6 left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Maybe another pair of 👀 from @jpveooys ?

@m7kvqbe1 m7kvqbe1 force-pushed the feat/timeline-styled-components branch from 85e35e2 to 273f1ca Compare September 25, 2020 10:24
@codeclimate
Copy link

codeclimate bot commented Sep 25, 2020

Code Climate has analyzed commit 273f1ca and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

View more on Code Climate.

@m7kvqbe1 m7kvqbe1 merged commit 09cad68 into master Sep 25, 2020
@m7kvqbe1 m7kvqbe1 deleted the feat/timeline-styled-components branch September 25, 2020 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: css-framework Package/code type Package: design-tokens Package/code type Package: react-component-library Package/code type Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate Scheduler Framework styles from BEM to styled-components
3 participants