commercetools / ui-kit Goto Github PK

The current page title of the website shows

We should use the ct favicon and name the page properly (e.g. UI Component library @commercetools)

Right now all of our fields reexport the static methods of the underlying inputs. We did this for convenience of our consumers, but it might actually add more confusion to have the same functions available from two places. It is also pretty inconvenient having to repeat the documentation of the statics in the fields when they're also available on the inputs already.

It might be a better idea to educate users of UI Kit to use the static methods of the inputs - even when using fields. I'm curious about the opinions of others.

This came up in #309 (comment)

We could (in a breaking change) remove the reexported statics methods of inputs from all fields.

Describe the bug
After fixing the naming for the latest new icons (#251), I noticed that most of them have invalid naming.

Current behavior
There are some icons that are named after the context where they are being used in the MC.

Expected behavior
Icons should be named according to what the icons itself represents, not the context.

If we decide to rename them, we should document properly which icons got renamed to what, to make it simple to migrate.

See proposal naming down below

Summary

All simple inputs support both error and warning states. In recent discussions with design we learned that we also need warning states on localised inputs. Just like their error messages.

Description

Given an e.g. is used I want to pass warnings just like errors which are rendered under the respective localisation in orange tone.

Alterantives

Showing the warning above the input itself. This however does not work when e.g. wanting to recommend (not enforce through errors) that a single localisation is unique across a set of entities.

Visuals

When viewing the deployed UI-Kit the README is only visible after a page refresh.

Reproduction:

1. open https://uikit.commercetools.com
2. switch to README tab

Expectation:
Readme is visible

Observed Behaviour:
"README.md was not added" is shown.

A page refresh at that point solves the problem.

This is likely to be a bug in Storybook itself.

We changed how MoneyInput.parseMoneyValue works, but we did not update the form examples, so they are erroring out at the moment. We need to pass the locale to these functions in order to fix the stories.

There is a problem with how Storybook displays Tables in README.md files. When a cell has no content, Storybook breaks.

a) We can go through all .md files of UI-Kit and add - to empty cells.
b) We can either try to fix this in Storybook (or the underlying lib). If we do that, we should go through all .md files and remove the - of empty cells in places where we're using it already.

I would prefer the latter if possible.

The READMEs contain examples of how to use the components. We did not yet upgrade the import paths in those examples.

We need to go through the examples and upgrade them to use the correct import paths.

Example:

- import MoneyInput from '@commercetools-frontend/ui-kit/inputs/money-input';
+ import { MoneyInput } from '@commercetools-frontend/ui-kit';

While we're at it, we can further improve the docs. The examples currently use value in onChange, but the components call onChange with an event:

- <TextField title="Username" value="foo" onChange={value => alert(value)} />;
+ <TextField title="Username" value="foo" onChange={event => alert(event.target.value)} />;

Description

I think silent reporters during test runs on CI are nice to not bloat the console when e.g. just stylelint'ing.

Maybe this is something nice to pick up on the side whenever somebody has some spare time. We have silent reporters setup in internal repositories so those could serve as a blueprint.

There was a wish that our MoneyInput should use thousand-separators. We were not able to do this previously as we were using <input type="number" /> to drive the MoneyInput.

Since we are now using <input type="text" />, we can add the thousand-separators.

Given that the user locale is en and the amount is 1000.34 then when MoneyInput gets blurred
it can format it to 1,000.34.

Given that the user locale is de and the amount is 1000.34 then when MoneyInput gets blurred
it can format it to 1.000,34.

However, we should not mess with the user-input while they type. Further, we should probably always parse 1000,34 and 1000.34 as 100034 cents. In other words: When there is only one separator, we should not rely on the user's locale to parse. We should always treat it as the lowest amount.

This umbrella issue keeps track of 3 things

• not formatting properly in Firefox on blur
• inputs look different in Safari
• ellipses are shown for the currency code even for short currencies like EUR

MoneyInput does not format on blur in Firefox.

It does work in Safari, but it looks a bit broken in Safari (the border gets truncated). The ellipses happens for all browsers though, but it should not appear.

Further, the currency gets ellipses in some cases. It should always be visible.

The ui-kit currently offers a component called TimeRangePicker which looks like this:

I would suggest to remove this component from ui-kit as it can be built pretty easily on top of existing ui-kit components, and it is only used once in the MC.

TL;DR: We want to add ACCEPTANCE_CRITERIA.md files next to README.md files which contain the acceptance criteria. They should be visible through storybook. They should be the single source of truth for how the component should behave. They aim to make it easier to learn about the intended behaviour of components.

Problem

When making changes to an existing component it is inconvenient/hard/a lot of work to collect the acceptance criteria it was built on. This makes changing the component hard as the original requirements are hard to find. Modifications can therefore lead to losing functionality which was part of previous acceptance criteria.

Current situation

Right now our acceptance criteria is located in Jira. When a new story is started (to build a new component), the acceptance criteria is collected there. After building the component, the acceptance criteria is not anywhere and just resides there. Sometimes when new things become evident along the way, additional information is mentioned in comments but doesn't become part of the acceptance criteria as it is not updated.

When bugs are found, new stories are created. Sometimes fixing bugs requires making decisions which change the original acceptance criteria. The original story is never updated and the additional acceptance criteria gets added to the new bug story.

To find the currently applied acceptance criteria, somebody needs to dig through all stories and bug reports of that component by searching for its name in Jira (and by following any "relates-to" links and by following subtasks of the stories). When a component was renamed in the process (e.g. while moving it from core to ui-kit) then Jira has to be searched for the old name of that component as well.

This makes it really hard to collect the current acceptance criteria which the component is built on, as

• the acceptance criteria is not colocated with the code (it is in Jira instead)
• the acceptance criteria is spread through multiple stories
• the acceptance criteria is not always updated after verbally agreeing on something

This can get really cumbersome and I therefore haven't seen us taking previous acceptance criteria into consideration when working on components. Nor do we write in-depth acceptance criteria to begin with.

Suggested solution

Colocating

We want to have one acceptance criteria document per component which serves as the single source of truth. For every component, aside from having a README.md we also want to have an ACCEPTANCE_CRITERIA.md next to it. This makes it easy to access while working on the code.

Making acceptance criteria visible

Having a markdown document tos how the acceptance criteria is nice for developers, but we also need to show the documentation to designers and QA. Our suggestion is to make the documentation accessible by displaying the ACCEPTANCE_CRITERIA.md from the codebase in Storybook by adding another tab next to the readme, like this:

Adding acceptance criteria when adding new components

The acceptance criteria would still start out in a Jira story when building a new component while the component is being refined. The PR which adds the component usually contains the documentation for it (the README.md file). Going forward, we should also add an ACCEPTANCE_CRITERIA.md file to the PR which contains the final acceptance criteria the component was built with.

Changing acceptance criteria when changing components

In case a bug is found, or the behaviour of a component changes (and the acceptance criteria with it), the new behaviour can be discussed in Jira or verbally. The PR which changes the code then needs to change the ACCEPTANCE_CRITERIA.md file as well. This aims to keep the ACCEPTANCE_CRITERIA.md and the actual behaviour of the component in sync.

All this aims to make it easier to learn about the intended behaviour of built components.

We came up with this suggestion after running into this problem. Thanks to @mariabarrena @filippobocik @antonboyko and @coolmilo who helped form this approach 💐

Problem

So far we didn't run any tests against the produced bundles. Our testing was spread to:

• manual testing inside storybook (which uses the Webpack configuration)
• regular .spec.js Jest tests which test the source-files, but not the final bundle (so the setup is different)

But none of these were able to test the final bundle we produce with Rollup. This left us pretty blind

• when updating dependencies related to Rollup
• when making changes to the build process

We also didn't have a proper way to ensure the styling of our components has no regressions.

Solutions

Testing the build

I suggest to test the produced bundle using a different Jest confing and different tests. Instead of calling these files .spec.js I would call them .bundle.spec.js to indicate that they are run against the bundle instead of the source files. Maybe we can rename .spec.js to .source.spec.js. Not too sure about the naming. Maybe just .bundlespec.js and .spec.js (or .sourcespec.js).

Tests against the produced build can verify that:

• the build is actually valid
• the build contains the exported customProperties, i18n and so on

Adding a Sandbox

We can add a sandbox (for lack of a better name) which is a web-app which uses the produced bundle just like a consumer would. We can do manual testing there.

Later on, this can be used as the basis for the Sketch export of our React components.

Adding Visual Regression Testing

We can further add Visual Regression Testing to ensure there are no visual regressions in our application. This will be immensely useful when making changes to the Rollup/PostCSS setup, but also when modifying components.

Getting there

To enable these solutions, we'd need to:

• add more npm scripts to run the different things
• add another jest config which runs the bundle-spec files
• set up Visual Regression Testing

State

I already experimented with all of the solutions above on https://github.com/commercetools/ui-kit/tree/df-add-sandbox. Some cleanup is left, but it works for the most part. I already showed it to @montezume to get some feedback, and he's on board!

I want to split that branch up into:

• adding a sandbox
• adding tests for the created bundle
• adding visual-regression-testing

Right now it's all entangled in one PR, but these are not related to each other. I will create different PRs soon to show the addition of these testing-setups step-by-step.

Todo list

For the current entangled branch, these things are left to do:

• split addition of sandbox (the localhost:8080 & wepback thing) and the addition of VRT
• enable having .bundlespec.js files spread across the folders instead of containing them within sandbox
• use two different test-util files (react-testing-library might work differently depending on each one)
• add custom eslint rules to ensure the .bundlespec.js does not import components directly (and uses the right test-utils)

We can let travis do the publishing to npm every time we push a new tag.

https://docs.travis-ci.com/user/deployment/npm/

Additionally, we can also decide to do "canary" releases (e.g. for master branch).

I will go with Travis:

• install deps
• run lint and tests

Description
I was looking for how we handle i18n messages in UI-Kit, but apparently there is no thing related yet but a command that does not work yarn i18n:build.

However I have some doubts regarding this.

• Is the UIKit intended to have an i18n folder? If that's the case, should we use the same Transifex project as the MC-FE?
• If we finally go ahead with this, should we just copy the scripts that we have for the FE here?

Additional context
I'm asking all of this because one of the points of the last UX/UAT review on channels was that the messages from LocalizedInputs (expand languages, hide languages) are not in transifex. So we have the whole view in DE except those two (we are using there LocalizedInput, MultiLineLocalizedInput)

Describe the bug
The <Toggle> component does not pass event to the onChange handler

Expected behavior
The <Toggle> component should pass event to the onChange handler, to make it easier to work with forms (Formik).

Once the new version of Formik is released, we should remove the setTimeout workarounds introduced in #102 and #108.

Describe the bug
I noticed why using the components that the READMEs are not up-to-date with the components. For example, some props are not documented, are wrong or inconsistent.

Expected behavior
The READMEs should always reflect and document the components APIs.

I would like to try out react-testing-library instead of using enzyme. In my experience it leads to more test coverage and more confidence while writing less code. The tests are also decoupled from the implementation that way.

If this is successful, I'd like to move all tests to react-testing-library and get rid of enzyme completely.

Additionally, our current tests are extremely slow in CI, some taking almost a minute.

 PASS   test  src/components/buttons/link-button/link-button.spec.js (52.084s)
 PASS   test  src/components/typography/text/text.spec.js (52.227s)
 PASS   test  src/components/inputs/number-input/number-input.spec.js (45.348s)
 PASS   test  src/components/switches/toggle/toggle-switch.spec.js (51.875s)
 PASS   test  src/utils/localized.spec.js (51.361s)
 PASS   test  src/components/buttons/ghost-button/ghost-button.spec.js (54.128s)
 PASS   test  src/components/buttons/accessible-button/accessible-button.spec.js (52.417s)

Source: https://travis-ci.org/commercetools/ui-kit/builds/424350666.

This might improve with react-testing-library as well.

Right now it's hard to test/check/see the results of the final rollup build.

It would be useful if we had an "example" application which consumes the final rollup build (the esm one). We could then start it manually after producing a build to verify that things still look right (manually at first). Over time, we could add visual regression tests for the components.

It would be cool if we could add <component>.prod.js to every component, which would render the component in multiple states on that example page (the prod extension name is up for discussion).

The advantage of testing the resulting rollup build is that we are testing the final result - and so we are testing our rollup build chain with it.

Problem

How we export and consume css variables leaves a bit to be desired...

Current situation

@import '@commercetools-frontend/ui-kit/materials/colors/base-colors.mod.css';

Possible way forward

postcss-custom-media has introduced importFrom and exportTo` functionality.

For example, for base-colors.mod.css (which is already generated from JS), we could instead have these generated to a json file, and then use importFrom to consume them.

I would propose doing a bit of research into this and making a spike out of it.

See issue.

Right now, percy is running babel on our built file. We can either solve this with a PR to percy, or we can deal with it through ui-kit.

I'd like to have a good and readable structure with emoji and so on to describe the release changes.

Background

Most of our bundle size is currently consumed with base64 encoded icon svgs.

Current situation

If a user does the following import in their application

import { AddBoldIcon } from '@commercetools-frontend/ui-kit'

their bundle is bloated with all of the Icons contained in ui-kit.

Definition of done

Users are able to import one icon, without having their bundle bloated with all of our icons.

Problem

At the moment we use a mix of props to basically explain the same two concerns, which adds unnecessary mental overhead when using our components.

The two properties we need to address

• Our multiline components can either have multiline text expanded by default or not
• Our localized components can either have all locales visible or not

At the moment we use the following props to express this:

Note that isDefaultClosed and isMultilineDefaultExpanded address the same concern. However they're worded differently and they even have opposite default states.

Similarily, isDefaultExpanded and areLanguagesDefaultOpened address the same concern. Their wording is completely different as well and they have opposite defaults too.

Solution

We can boil the four different prop names down to just two different prop names; one for each concept.

I'm not too enthusiastic about these names and I'd welcome suggestions 👂

As pointed out in #18 (comment), we can likely use maxWorkers to reduce our CI run time.

This application uses the presets we usually use to build the MC apps. However, the setup is a bit different and we thus incur dependencies of things we don't need (see #115) and are limited in what we can configure (it's not possible to turn module mocking of .css files off in Jest for example, as we can't overwrite the preset).

The presets are a good fit when working on MC apps, but not such a good fit for the UI Kit. Using the presets limits the flexibility (I ran into issues with the Jest setup), and incurs unnecessary dependencies and peer dependencies.

The exact dependency requirements listed in the presets can also prevent us from upgrading other dependencies (as it happened in #111).

Of course the presets have some advantages, but they seem to not outweigh the downsides we are facing. Again: Presets are great for MC apps, but problematic for UI Kit as it isn't one. I would like to have more flexibility in our setup of UI Kit, as all the consumers should care for is the final build anyways.

I propose to get rid of:

• @commercetools-frontend/babel-preset-mc-app
  • configures transforms we're not using (e.g. GraphQL)
• @commercetools-frontend/jest-preset-mc-app
  • has been getting in the way of the visual regression testing setup
  • it configures things we don't need (like GraphQL)
• @commercetools-frontend/mc-scripts (maybe, we only use mc-scripts extract-intl)
  • it forces a dependency on graphql-tag which forces graphql, both things we don't need

I would use a custom configuration instead.

I would keep @commercetools-frontend/eslint-config-mc-app.

Right now we only use Constraints and Spacings to create a sort-of grid. We should add a proper grid to the UI-Kit.

https://github.com/commercetools/ui-kit/issues/templates/edit

Right now the ui-kit hosts some images for the Merchant Center which are unrelated to the ui-kit itself. We should move those to a more appropriate place like commercetools/merchant-center-application-kit.

We currently include styled-components just for its keyframes feature which we only need for one component. We should get rid of styled-components and find a different way to implement the one component using it (CollapsibleMotion).

We should set it up properly with a bot and so on.

Not a high priority but it would be nice to have. We can also use it for the other public repos.

There should be a specific domain for documenting this. For example: https://contribute.commercetools.com

Some noteworthy examples:

• https://contribute.meteor.com/
• https://code.facebook.com/cla
• https://cla.opensource.microsoft.com/
• https://cla.developers.google.com/

I've been thinking about this for a while and I think the ui-kit it would be a perfect place to start using types.
I believe the most suitable option for the ui-kit is to use Typescript, as it's a public library and the ts adoption/support is much better, which makes it easier to consume.

Besides the advantages of typing the components, we can also leverage the types to auto-generate certain things, like props (instead of manually maintaining this, which causes inconsistencies). For example: https://www.npmjs.com/package/react-docgen-typescript.
I opened another issue that points to that problem: #319

Let's think about this as a long-term solution, we don't need to jump on this right away.

The Storybook available at https://uikit.commercetools.com/ should represent the version available in the latest release, instead of the latest development stage to avoid confusion and bugs.

This was not possible the last time we checked with Netlify. We might need to change our deployment setup for this.

Once we fixed this, a related idea is to have a second deployment with a dedicated URL which shows the latest stage of UI-Kit, e.g. https://uikit-latest.commercetools.com/ or https://latest.uikit.commercetools.com/.

Right now quite some of our components auto-generate an id in case the parent doesn't pass one in so that we can hook up labels to inputs. Unfortunately the code to do so is quite spread around the component. Usually we do

import createSequentialId from '../../../utils/create-sequential-id';


// somewhere at the top
const sequentialId = createSequentialId('date-field-');

export default class Foo extends React.Component {
  state = {
    // We generate an id in case no id is provided by the parent to attach the
    // label to the input component.
    id: this.props.id,
  };

  static getDerivedStateFromProps = (props, state) => ({
    id: do {
      if (props.id) props.id;
      else if (state.id) state.id;
      else sequentialId();
    },
  });

  render () {
    return <input id={this.state.id} />
  }
}

That's a bit much setup and it's spread across different sections of the component.

We could improve this by creating a HoC which ensures there is an id. The usage would then look like this instead:

class Foo extends React.Component {
  render () {
    return <input id={this.props.id} />
  }
}

export default withId(Foo)

The downside of this is that all the components would be wrapped in a HoC, which makes it less easy to see them in react's dev-tools.

We are currently only using the keyframes feature of styled-components, and only for a single component. It doesn't make much sense to bundle all of styled-components for this.

We should make styled-components a devDependency and restrict its usage to stories of storybook!

Our current buttons implementation look enough (they are in UI Kit, they are implemented and work). But, if you take a closer look, they actually hold wrong concepts and different props.

1. Concepts

We misuse some concepts about buttons, like:

• A PrimaryButton was supposed to be a primary ACTION button, the action we would suggest our users to take first, that is. But it was extended with a prop called type, and now it can be urgent.

• A SecondaryButton in the other hand, is supposed to be a secondary ACTION we would suggest our users to take. But we implemented a prop called theme, and it can be blue. (blue is actually used for information in the application)

Quick example from Apple design system:

• A GhostButton can be used to trigger tertiary actions. But we don't even use it. It could be simply removed (we actually use it once, but we can revisit it since its not necessary there)

• A FlatButton is what we use for tertiary actions. We currently have types for this, and actually our "LinkButton" has same styling as the "secondary" type.

And so on and so forth...

2. Props naming (theme, tone etc)

We can see in the mind map below, we use inconsistent names for props across our buttons. That makes the understanding of them quite complicated. What we want in the end is just to achieve some different style (exception for a link button or submit button, which we can have different component for).

To help illustrate all of this topics, I made the mind map below:

❌ Current approach:

✅ Proposed approach:

Any thoughts? objections?

The FieldErrors component knows how to handle some errors: missing, negative, fractions.

When writing consumer-side form code this connection to FieldErrors is not immediately obvious. If we exposed those keys it might be clearer. We could export FieldErrors.handledKeys.MISSING, FieldErrors.handledKeys.NEGATIVE, FieldErrors.handledKeys.FRACTIONS.

cc @islam3zzat

Background of inputs should stay white when focused.

We basically need to re-apply the changes from: https://github.com/commercetools/merchant-center-frontend/pull/5346 which already had fixes for these.

When using SelectInput, and when it had a value before, then the value is not removed when the value prop changes to undefined.

@islam3zzat notified me about this, and we looked into it together. He will provide a fix for it.

Problem

Right now the UI-Kit requires consumers to provide some style, otherwise the ui-kit looks pretty ugly.

For example, when an application only includes the PrimaryButton and the MoneyInput, then the result looks like this.

The reason the ui-kit looks unstyled is because the UI-Kit currently requires parents to apply these styles:

• @commercetools-frontend/ui-kit/materials/reset.mod.css
• @commercetools-frontend/ui-kit/materials/grid.mod.css

By doing

import '@commercetools-frontend/ui-kit/materials/grid.mod.css';
import '@commercetools-frontend/ui-kit/materials/reset.mod.css';

For the MC, this is currently done here.

Notice that we currently also need to import these styles into storybook, otherwise the ui-kit would look unstyled. Additionally, the reset.mod.css uses features like @import and var(--color-black);. This is bad because it requires the consumers to have the tools ready to transpile these features. You can see the build-output at unpkg.com/@commercetools-frontend/[email protected]/materials/reset.mod.css.

Ideal solution

Ideally, the ui-kit would be self-contained. All components of the ui-kit would their full styles applied. The ui-kit should not add any global styling to the consumer, nor should it require the consumer to apply any global styling.

This is a bigger step to achieve, as we'd have to add all global overwrites of reset.mod.css to each component (e.g.font-family to all components).

Intermediate suggestion

Because it is quite a bit of work to move the internal styles around, we can take an intermediate step. We can include reset.mod.css and grid.mod.css into index.js of ui-kit. The global will then get applied automatically, and consumers no longer need to import styles from ui-kit only to apply them to the body afterwards. This also lifts the inconvenience of ui-kit consumers needing to have a setup which can parse custom-properties (color: var(--color-black);) and @import statements. This also allows us to remove the import from app-kit and from storybook.

This intermediate suggestion further already heads in the right direction - as the parent on longer needs to add any styling. We still apply global styles, but moving those to component-styles will be an internal change then.

Notes

This is a lot of text, but reaching the intermediate solution shouldn't actually be that much work.

Right now, when some of our components need to use other components they use relative imports and reach for that component directly.

As we're exporting all components through index.js, it would also be possible to always import other components from index.js (if they are public). This would allow us to use a single import for ui-kit components when when building other ui-kit components.

Example

// money-field.js
import Constraints from '../../constraints';
import Spacings from '../../spacings';
import FieldLabel from '../../field-label';
import MoneyInput from '../../inputs/money-input';
import FieldErrors from '../../field-errors';
import { VerifiedIcon } from '../../icons';
import { Text } from '../../typography/text';

could become

// money-field.js
import {
  Constraints,
  Spacings,
  FieldLabel,
  MoneyInput,
  FieldErrors,
  VerifiedIcon,
  Text
} from '../../'

• This would keep our components unaware of each others location
• It means that we don't need to worry about relative paths so much
• It makes it easier to import an additional component
• It indicates which components are public and which components are supporting components of the current one

This is an idea and proposal to simplify the usage of several things.

The problem

Building a view using UI components is great, it makes it easy and fun. However there is one little issue that is kind of annoying and that you end up doing all over again: duplicated messages.

For example, when you build a form you usually need a Cancel and Save button. So you usually end up creating messages for those buttons because the UI components require you to pass a label.
This happens for other things that are spread all over the different views.
And of course you end up having A LOT of duplicated messages to be translated, when 99% of the cases might not be necessary.

The proposal

It would nice to have some sort of "preset" components that are translated and everything within the ui-kit, so that as a consumer we don't need to care about all those messages again.

For example:

• <CancelButton>
• <SaveButton>

...and whatever other components might be useful.

Thoughts?

We think this is due to the setup of Storybook, especially postcss-color-mod-function.

Related: csstools/postcss-preset-env#48

It would be cool to add some badges (Prettier, npm install size, latest version on npm, ...).

We are seeing problems with our current table. We need to adapt the table.

The tables are next on our list and we are currently gaining all requirements. The topic will become clearer soon.

Problems with Table

• requires some cells to use tethering hack
• has unnecessary virtualization
  • prevents browser search from finding cells
  • we don't display enough rows to warrant having virtualization in the first place
• Does not update when inner components rerender (setState within them)
  • makes it hard to use table for forms or for dynamic stuff in tables (e.g. Collapsible Panel)