Skip to content
This repository has been archived by the owner on Feb 29, 2024. It is now read-only.

Questions for Thinkmill

Jump to bottom
Jonny edited this page Oct 10, 2019 · 22 revisions

To discuss with Thinkmill

Are the following assumptions correct / best-approach?

Tokens

  • Currently the /brand/* folder's index.js files provide one global token object. Component keys have been used to separate 'local' styling concerns. I understand these settings should be separated into 'local tokens', unique to each component package. '
  • We need some help understanding how local tokens have been set up. Are local tokens just exported variables within the React components, or is there a /brand/*/index.js file structure missing from the solution?

Tokens concept and use case has been explained. Inside a component we style as per normal. Externally we store global settings in tokens (colours, type, layout etc). Transformers help us to get what we need for our consuming environment (ie. rem units and hex colours for web). Token implementation updated since, and we're all on the same page now 👍

  • Tints: have implemented a tint(color, amount) function via a simple color dependency. Perhaps there's a better way?

New implementation of tints has been implemented via the web transformer. Each colour key gets a tint key to use, e.g. Tokens.COLORS.tints.primary50 (50% tint of primary)

  • Relative units: we must use relative units to meet a11y requirements. After converting token values to rems I began thinking... Why not store in px and translate to rems when consuming inside components? Rem values are useless outside of the web.

Use rem units internally, apart from border/outline widths (strokes). We store px based (minWidth) breakpoint values in LAYOUT token`

Testing

  • How to write test scripts in Jest and Cypress
  • How to design tests, what to think about etc.

React Context

  • We've used Context to pass parent props to sometimes deep nested sub-components (e.g. Form)

Great. Good stuff. High-five! We will use this more, instead of prop-drilling via the cloneElement() pattern

  • Also used to power isKeyboardUser mode (see Core)

We will implement this slightly differently. Flipping the approach, see below in Core.

  • Best way to allow specific props to be set on a parent component (for all children to consume) and individually on children; without defaultProps getting in the way. e.g. Form and TextInput... Form context has no default, and TextInput checks for size prop passed from Form context, then for props passed to TextInput itself. Is this the way to handle this pattern?

If default props get in the way you don't set them; set them in code. There is no way to determine if a prop is set via implementation or via default props. Generally we want to get the value via the element's prop, then from Context, then by a default set in code. e.g. const textInputInline = inline || formContext.inline || false;

Core

  • Styling: we've extended the core package's Theme component by adding a Core sub-component. Core provides global styling (resets, typography etc) within the Theme.Provider environment using the Emotion Global component.

Many of these typographic resets will move to a new Body component.

  • Rems: Currently setting document (<html>) font-size to 62.5% (10px), giving us a px->rem conversion that's easy to work with. <body> resets this to 1.4rem giving us our (unfortunately small) 14px base font size. This is set in tokens. I know this 'reset to a workable 10px' method is greatly debated on the webs.

We will remove the 10px reset. Document font size will be the browser default (16px). The 14px body reset likely to move to the Body component. This will mean we may need to set fontSize for some components.

  • isKeyboardUser mode: uses Context to provide child components access to this state (triggered on tab press, as per GUI2.0 approach)

We will flip the approach here. There will be a isMouseMode flag, set as a class on body element. This flag will be set by default, and removed when tab key is pressed. The logic to power this will be provided by a simple vanilla JS script; rather than within our React environment. The result of this is that we can globally style :focus outline, and override to remove when .isMouseMode is found. This flipped approach is safer as we can be certain we're only removing default focus outline styling when necessary.

  • Removed the xs breakpoint (from tokens). XS provided as the viewport width < sm (ie. breakpoints.sm - 1 if required). Facepaint is mobile first and takes the default (xs) setting as the first value passed in facepaint array.

I didn't receive any complaints from TM. Assuming this is ok.

Fonts

  • We have stored the font names/weights in tokens and pulled these through in Core
  • How do we bring these into our solution for testing?
  • Assuming these will not be published due to licensing restrictions. Need to document where devs should to save the webfont files, how to deploy etc.

TYPE token takes care of this. The body font stack fontFamily and brand fontFamily name and @font-face (file paths) are set in this token. The web transformer gets us the CSS (@font-face) output we need.

Form

  • One package (form) comprising of the bitsy little layout/styling components required to compose a form
  • Form input components are found in separate packages (eg. TextInput, FormCheck, Button, ButtonGroup etc.)
  • Should we auto-generate form element IDs? If so, also expose a prop to override this?
  • A single TextInput component has been built to render Text inputs, Selects and Textareas. These share the same props (size, width, invalid, tag etc.)
  • Select dropdown arrow/caret icon rendered via data-uri SVG background using a simple SVG->data-uri dependency. Approach also used for list bullets.

Symbols/Logos

  • Symbols and Logos have been combined into the one component Symbol and follows a similar approach to Icon (created by TM)

This will change. Icons will also, as we have to consider code-splitting. The TM built Icon doesn't provide that yet.

  • As with Icon... is aria-label supported well enough by screen readers? Intopia have previously told us to use ‘sr-only’ technique for alt text

  • Multi-brand logos: The appropriate brand logo will render depending on the current theme context. Implemented via a new brand key (string) in the tokens. Is there a better way?

Yeah this is not the right implementation approach. To discuss further. 'Tokens' was mentioned 🤔...

  • Is 'Multibrand' the right name? Maybe we can drop the 'Multi' from the component name and just call these Logos 'Brand...' e.g. <BrandLogoLarge />?

LogoLarge would be a better name. We'll get to this soonish.

Icons

  • Would it be more conventional to name these components 'Icon' followed by the name ie. IconHouse, IconInfo, IconAlert (rather than HouseIcon, InfoIcon, AlertIcon)
  • As with Symbol... is aria-label supported well enough by screen readers? Intopia have previously told us to use sr-only for alt text

Alert

  • Should the alert box remain in the DOM after closing, or simply CSS hide? Legacy system consideration?

  • Should we be using the Button component or just a regular button for the close option? Also with Modal component.

Dom has previously told me to be 'atomic' and reuse the components we already have

Badge/Label

  • Should we just combine these into the one component? They are 99% the same. Badges can be links, Labels cannot. tag and appearance props would allow us to consolidate these. FWIW: labels/badges are rarely used.

List

  • List appearance nesting issues (props overriding children)
  • Have issue in docs with children prop mapping not working correctly so nested colors are incorrect

Will move to Context rather than pop-drilling approach. Will likely fix the docs issue.

  • How to handle children proptype checking. Having issues checking via PropTypes.shape

Modal

  • Whether separate modal header and body components are necessary

Table

  • Table component currently only provides the parent wrapping. Row and cell (children) elements are composed with standard html elements. Are there any benefits in abstracting these standard html elements into React components?

  • Responsive table (wrapper) element enabled via prop. Maybe this should be the default? noResponsive prop to disable?

  • Table wrapper class table-responsive doesn't feel right. Used by Panel component styling; responsive table styling reset when placed inside Panel.

We will use a prop to provide the style reset

  • .table-highlight class doesn't feel very react-esque

Button

  • Icons: Buttons take Icon children. How are we best to style these? I've taken a leaf from Atlaskit's book, using iconAfter/iconBefore props. Simple enough. Thoughts?

  • Our approach with this has been to maximise the use of tokens; but since Button is quite complex, it can take numerous prop options (incl. size, appearance, soft and block) plus requiring :hover and :active state styling, it has made for a rather verbose token structure. Verbose tokens yes, but much simpler component code (and the power of publicly exposed button styling). Thoughts?

Less of a problem when new token structure is implemented and the style values are brought inside the component.

  • Best way to render gap between adjacent inline buttons? InlineWrapper wrapper component? Should that add marginLeft styling to each adjacent sibling?

ButtonGroup

  • Had a thought. This could really easily be merged into the FormCheck component; provided via prop type="button" (adding to radio and checkbox types). Thoughts?

Maybe down the track. ButtonGroup has a Button dependency, FormCheck does not (currently). Probably best as its own component.

  • Currently setting button's borderRadius and border styling with CSS. Consider doing this programmatically, iterating children and applying borders as needed? Similar to InputGroup question.

InputGroup

  • We originally implemented this by overriding children sub-component CSS styling (border radiuses, borders etc), but refactored to use JS logic (first/last props) to style internals by knowing child index
  • :first-child SSR warning. How to get around this?

FormPod

  • Is the 'slots' method the best approach? (see FormPodFooter and FormPodActions)
  • Best way to provide FormPodContactList (passing data to items prop)?

Breadcrumb

  • Separator icon (>) as embedded SVG icon, or background image (data-uri) as with select carets and list bullets?

Panel

  • Styling children: Is the cloneElement() approach the best way to style Panel's children (i.e. PanelHeader, PanelBody & PanelFooter) when access to a parent prop is required? We could add a className on the child components and select them via child selector in emotion, use the styled component approach to target the child component (see https://emotion.sh/docs/styled#targeting-another-emotion-component) or use Context? Question applies to many components.

We will generally use Context to pass values to children (see above)

  • Do we need to export TableHeader and TableFooter components? Can we just take props for the header title and footer text?

  • Table wrapper class table-wrapper doesn't feel right. Panel refers to this selector.

We will pass a prop to provide this special styling, or likely make a separate PanelTable component which extends Table.

Helpers

  • SrOnly and SrSkipLink (simple) components are found in the one accessibility-helpers package
  • Have built a Hide component, can pass it an isShow prop with boolean facepaint array values. Have played around with using this to append CSS display:none to children's emotion CSS object. Keen to run it past you.

Testing/Example environment

  • Might need to rethink the sidebar so we have access to the viewport 'edge-to-edge' (i.e. more accurate testing in XS)
  • Example environment should really utilise our Core and Template components (i.e. more accurate environment for testing... typography, background colour, responsive page wrapper etc.)

Emotion

  • Should we just always pass our Emotion CSS objects through facepaint mq() even if the passed values don't include array values? This would ensure if a token is updated to use an array value it would style responsively as expected. Otherwise our component(s) code would have to be updated, bumped and re-published to support the token update 🤕

Not a problem, tokens aren't what I thought they were.

React

  • Props
    • Is there a way to differentiate between a prop passed or a default prop
    • Extent of proptype checking, is it really necessary to prop check simple styled components
    • Should we be using a custom proptype checker to test that children are the correct components
  • Transitions
    • recommended library/method for doing them
  • Focus handling and trapping
    • recommended library/method for doing them
  • Best practise for using SVG's as backgroundImageUrls
    • Need to change svg color based on props
    • currently using svgtotinydatauri package to help with this
  • Controlled Components
    • Best practise
    • is it safe to assume that an onChange function will always be passed? - currently have a pattern to handle no function being passed, is this correct?

General

  • How is the <head> tags etc set up. Re meta tags (e.g. responsive viewport meta tag)... is this a concern of the GEL app at all, or the responsibility of the developer to implement? How do we handle this in the example environment?

Guidelines: Are the following coding guidelines correct...

  • Use rems except for... border-width, border-radius, outline, outline-offset, @media queries (until we discuss)

  • Boolean props follow the isPropname convention (currently except for disabled and invalid props which are passed as html attributes, not sure if we should use isDisabled and isInvalid, then typecheck and render as html attributes)

We've decided to drop this. No 'is'.

  • Typecheck (propTypes) every component's props, whether its a simple style component (in src/styled.js) or a child component that receives props from a parent. Check all.

    • Provide simple concise description (comment) for each propType declaration
    • Supplement with any further helpful usage details as appropriate
    • Don't mention default values within these comments, these are automatically rendered in a props table within dev docs

  • Group multiple style objects, nest within a variable to keep styles tidy, as appropriate

Incorrect. Inline all the styles, unless there is a considerable amount of styling concern ie. Button

  • Props accept an array value to set values per breakpoint, as appropriate

  • 'Tokenise' all styles providing 'characteristics of design'

Incorrect. This isn't what tokens are. Misunderstood, soz.

  • Provide a tag prop to set the component’s outer element if beneficial

  • Consider checking element type when cloning child elements, so new props aren‘t unnecessarily passed to all child components

  • Use Context to share parent props, rather than deep ‘prop drilling’, as appropriate

Correct

  • Use HTML elements (e.g. checkbox/radio) and CSS selectors (e.g. :checked) to handle the styling of component state. e.g. Checkboxes, radios, switches etc., rather than relying on React state to visually style the state. This will help us power our legacy (non-react) version.

Incorrect. We render (hidden) html checkboxes/radios because it's good practice and helps a11y users etc., but don't render active/checked styling based on the html state. Our inputs are to be 'controlled components'. We use react state to style our custom active toggle marks (🔘/☑️ etc).

Some issues we've come across

  • Packages added via bolt/yarn add ... are making package.json fail CI (due to formatting)
  • Symbols package docs not rendering props table
Clone this wiki locally