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

Changes for DD pd to use DS components #2391

Closed
wants to merge 8 commits into from

Conversation

lucymonie
Copy link
Contributor

@lucymonie lucymonie commented Mar 10, 2020

Why are you doing this?

I have completed a spike focused on the design system (DS). The aim was to try implementing components and foundations from the DS in section of the checkout.

Trello Card

I chose the direct debit progressive disclosure section to update.

Upgrades and installations I needed:

  • Node 12
  • @guardian/src-text-input
  • @guardian/src-button
  • @guardian/src-checkbox
  • emotion-theming

Already installed:

  • @emotion/core (css, jsx)
  • @guardian/src-foundations (space)

What I used from the DS

Colour: the colours came via theme providers, which are made available through emotion-theming. The usage is very clearly described in the DS docs and was easy to implement. I used this in the ‘Confirm’ button.

Typography: this came from foundations and was clearly documented and straightforward to implement. I used the typography for the section heading.

Spacing: the spacing between the input fields is clearly documented but at this point I struggled a bit getting it to work as expected. Eventually I realised that I needed to import ‘jsx’ from @emotion/core to make the spacing work. Then I needed to upgrade node and delete my node_modules and run yarn install a few times to get it to compile. The dependencies could be more clearly specified in the developer section of the DS docs.

Emotion: I used emotion to style the form layout. This was very clearly documented in the DS docs and was straightforward to implement. I went with the template literal style and found it readable and easy to interchange with the sass files as I could just paste in the styles from the sass file.

Issues I encountered

  • Chrome grid overlay link is broken (I have mentioned this to Simon)
  • I had some trouble setting up emotion because of missing jsx - this could be better explained in the DS, or a pointer to the relevant part of the emotion docs might be useful
  • The checkbox text options force you to use a label if you want supporting text, but not just supporting text. I have raised this with Simon and he has asked me to submit a feature request for this
  • The error summary is not a supported element as yet - it looks pretty weird next to the new DS elements. I have raised this with Simon so it’s on his radar. He thinks they will recommend keeping it but there’s no guidance yet in the DS
  • I found the suggested spacing at the end of the form (DS says it should be 96px) looked massive and out of keeping with the rest of the form, so I used less. Is this a judgement call thing? On revisiting the documentation, it says either 48px or 96px, and I used 48px so this is fine.
  • The look of the DS components is a bit different from what we have now, and we wouldn’t want to merge in the work I’ve done unless the rest of the checkout was also updated. It might be worth considering doing this work ahead of the large checkouts overhaul so the DS is somewhat embedded as a supporting piece of work.

Imports

I have added @emotion/babel-preset-css-prop to the project, plus the following code to the .babelrc

{
  "presets": ["@emotion/babel-preset-css-prop"]
}

This means that we can use the following imports in all files to make emotion work:

import React from 'react';
import { css } from '@emotion/core';

Actions

  • Submit feature request for checkbox with supporting text but no label
  • Try to get a better understanding of how strict the DS guidelines are
  • Further exploration of template literal vs object style with emotion
  • Share findings with support-frontend DS group (Tuesday)
  • Raise the possibility of rolling out the DS in subs checkouts ahead of the larger checkouts work
  • Look for documentation on rolling out emotion (use of jsx import from @emotion/core etc)
  • Add emotion config to support-frontend to smooth the way for working with DS and emotion in the codebase

Screenshots

Screen Shot 2020-03-10 at 20 47 23

Screen Shot 2020-03-10 at 20 47 57

@lucymonie lucymonie added the Do Not Merge May still be WIP or unstable label Mar 10, 2020
@jfsoul
Copy link
Contributor

jfsoul commented Mar 11, 2020

💯

On strictness of the DS, maybe this is something we can chat to design about as well. I'm sure it will come down to judgement calls, depending on the context.

import React, { type Node } from 'react';
/** @jsx jsx */ import { jsx, css } from '@emotion/core';
import { headline } from '@guardian/src-foundations/typography';
import { palette } from '@guardian/src-foundations';
Copy link
Contributor

Choose a reason for hiding this comment

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

We've found that importing the entire palette can bloat your JS bundle as you're loading lots of colours that you're not using. We recommend importing only the colours for the context in which you're using them from the /palette folder:

import { border } from '@guardian/src-foundations/palette'

const showFormSection = css`
    border-top: 1px solid ${border.secondary};
`;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

>
<span className="component-direct-debit-form__cta-text">{props.buttonText}</span>
<div className="component-direct-debit-form__arrow"><SvgArrowRightStraight /></div>
</Button>
Copy link
Contributor

Choose a reason for hiding this comment

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

Did the button icon API not work out for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah rubbish! No - I just missed it 😳

I'll amend to use it.

@SiAdcock
Copy link
Contributor

This is great, thanks so much for the spike and all the detailed feedback @lucymonie 🙏

I needed to import ‘jsx’ from @emotion/core to make the spacing work

Was this just for spacing, or was it for usage of the css prop in general?

As an alternative to the jsx pragma, you can use Emotion's Babel Preset, which uses Emotions's jsx wrapper in a more frictionless way. Note this will call jsx, rather than Preact's h() function directly, in all modules. Either way, you make a good point, we should help new Emotion users get up and running faster.

Share findings with support-frontend DS group (Tuesday)

Oooh, that sounds fun! 😄 Please can you let me know what comes out of it?

@jfsoul said:

On strictness of the DS, maybe this is something we can chat to design about as well.

Is this in reference to how the design system components look different to your current components? Note that you can override styles in design system components to bring them into line with your existing look and feel in the short term. Let me know if this would be useful to you and I can quickly draft up some docs.

@@ -5,6 +5,11 @@
.*target/.*
.*/__tests__/.*

[untyped]
Copy link
Member

Choose a reason for hiding this comment

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

What is different about this compared to the ignore list above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe if it's ignored you can't import it, whereas if it's untyped it's given the type 'any'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do Not Merge May still be WIP or unstable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants