Restructure styles, improve code and docs by vojtechszocs · Pull Request #150 · kubevirt/web-ui-components

vojtechszocs · 2018-12-20T19:37:13Z

Fixes #137 and #2 (with regards to better documentation).

Styles

Adopting the BEM (Block Element Modifier) convention for CSS classes along with the kubevirt prefix to avoid potential conflicts with the consuming application.

For example, a fictional FancyForm component could use the following class names:

kubevirt-fancy-form
kubevirt-fancy-form__submit-button
kubevirt-fancy-form__submit-button--disabled

@rawagner Each BEM "fragment" (block, element or modifier) should comply with [a-z-]+ regexp, using __ and -- to separate the block and modifier. See also this StackOverflow answer for additional context.

Using stylelint-selector-bem-pattern / postcss-bem-linter to ensure that all sass/components/**/*.scss stylesheets conform with the BEM format.

The BEM block (or "component" in postcss-bem-linter lingo) is automatically inferred from the file name, e.g. all selectors in sass/components/Form/form-group.scss should start with .kubevirt-form-group. Different BEM blocks should go into different stylesheets.

Q: What about nested CSS selectors like .kubevirt-form-group .dropdown > .dropdown-menu ?
A: These should be OK from BEM point of view. In short, each block represents an independent entity, so nesting CSS selectors under a block shouldn't affect other blocks. That said, try to avoid nested CSS selectors if possible - try to keep things simple.

PF-React overrides are placed in sass/patternfly-tweaks directory. These should be treated as temporary until being backported to the corresponding PF-React npm package.

Check out "Stylesheet file structure" in docs/DevGuide.md for more details.

Code

Using the import/order ESLint rule in hopes of reducing the chaos in module imports. Anything that isn't project-related should be imported first, followed by a newline and then followed by anything that is project-related:

import React from 'react';
import { get } from 'lodash';
// --- newline ---
import { ListFormFactory } from '../Form/FormFactory';
import { getName } from '../../utils/selectors';

Added dependency on classnames to reduce complexity when computing the space-separated React/JSX className prop, for example:

<FormGroup
  className={classNames('kubevirt-form-group', {
    'kubevirt-form-group--no-bottom': horizontal && field.noBottom,
    'kubevirt-form-group--help': !horizontal && hasValidationMessage,
    'kubevirt-form-group--no-help': !horizontal && !hasValidationMessage,
  })}
>
  {/* stuff */}
</FormGroup>

Docs

Added a docs directory with following content:

Setting up development environment
Developer guideline
Building & publishing
Travis CI

Updated the README file with links to above documents.

Dependencies

Bumped the @patternfly/react-console version to pull in the fix for unnecessary peer deps, reducing the amount of warnings upon yarn install.

Miscellaneous

Removed HelloWorld and ButtonWithIcon components after a previous discussion with @rawagner.

Checkbox, Dropdown, Integer, Text and TextArea components now have their id prop marked as required for better end user accessibility (a11y).

Updated all component test snapshots to match the newly introduced CSS class names.

Fixed the console error when running tests (related to prop types validation):

PASS jsdom-env src/components/CreateDiskRow/tests/CreateDiskRow.test.js
  ● Console
    console.error node_modules/prop-types/checkPropTypes.js:19
      Warning: Failed prop type: The prop `LoadingComponent` is marked as required in `CreateDiskRow`, but its value is `undefined`.
          in CreateDiskRow

Also, when passing React components as props, I'd highly suggest using PropTypes.element instead of e.g. PropTypes.func. (Didn't make this change now to avoid potential breakages.)

coveralls · 2018-12-20T19:40:17Z

Pull Request Test Coverage Report for Build 592

55 of 57 (96.49%) changed or added relevant lines in 7 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.8%) to 86.636%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/components/Form/FormFactory.js	1	3	33.33%

Files with Coverage Reduction	New Missed Lines	%
src/components/Table/EditableDraggableTable.js	1	65.41%

Totals
Change from base Build 456:	0.8%
Covered Lines:	1409
Relevant Lines:	1552

💛 - Coveralls

vojtechszocs · 2018-12-20T19:41:07Z

I've also noticed that we're doing things like:

Integer.defaultProps = {
  value: undefined,
  defaultValue: undefined,
  // other stuff
};

in order to satisfy the react/require-default-props ESLint rule. To me, those explicit undefined default prop value declarations seem redundant, but there doesn't seem to be a work around this at the moment. Check out the discussion here for more context.

rawagner

@vojtechszocs thank you for explaining BEM! It was really helpful. Can you please take a look at my review comments and rebase ?

rawagner · 2019-01-03T11:40:00Z

sass/components/Form/dropdown-group.scss

@@ -0,0 +1,3 @@
+.kubevirt-dropdown-group .dropdown-menu {


I would prefer to call this .kubevirt-dropdown__dropdown-group .dropdown-menu and move it to sass/components/Form/dropdown.scss

sass/components/Form/dropdown.scss contains styles for the DropdownButton component.

sass/components/Form/dropdown-group.scss contains styles for the ButtonGroup component, which wraps the DropdownButton component.

<ButtonGroup className="kubevirt-dropdown-group"> <DropdownButton className="kubevirt-dropdown"> {/* stuff */} </DropdownButton> </ButtonGroup>

The motivation behind this was to impose style granularity per logical React component.

However, looking at sass/components/Form/form-group.scss there's some redundancy - notice the width: 100% rule:

.kubevirt-form-group .dropdown > .dropdown-menu { width: 100%; display: none; z-index: 1001; }

I'll fix this redundancy by moving the width: 100% rule into sass/components/Form/dropdown.scss as suggested.

thanks @vojtechszocs , sounds good to me. Lets remove the redundancy.

sass/components/Table/editable-table-actions.scss

atiratree · 2019-01-03T12:12:31Z

src/components/Table/TableFactory.js

 import { Button, ButtonGroup, Alert } from 'patternfly-react';
-import EditableDraggableTable from './EditableDraggableTable';
+
+import { default as EditableDraggableTable } from './EditableDraggableTable';


does this approach have any benefits or is this just a consistency issue?

Just a consistency issue.

atiratree · 2019-01-03T13:33:29Z

src/components/Table/InlineEditRow.js

 //   Stateless function components cannot be given refs. Attempts to access this ref will fail.
 // eslint-disable-next-line react/prefer-stateless-function
-class InlineEditRow extends React.Component {
+export class InlineEditRow extends React.Component {


What is the preferred approach? using default exports or exports?

For our React components, we should use named exports (i.e. not default exports).

That's the convention we started with, so we should stick with it consistently.

$ grep -r 'export default' src/components --include="*.js"

One exception is the EditableDraggableTable component, which uses a HoC function:

export default DragDropContext(HTML5Backend)(EditableDraggableTable);

and I didn't like having neither _EditableDraggableTable for the actual component nor redefining EditableDraggableTable variable (ESLint will warn about that).

So unless it really hurts readability/complexity, I'd always favor named exports in modules representing our React components.

vojtechszocs · 2019-01-04T19:40:21Z

Thanks guys for your review. @rawagner please see my replies and let me know what you think.

I'm going to add one more commit to this PR - improve React component file structure validation & bring test coverage of tools/validations to 100%.

@rawagner also, please see my comment on PR #101 - validations are used in production builds and should deserve to be treated as such.

vojtechszocs · 2019-01-04T20:44:53Z

Example of componentFileStructure validation run:

vojtechszocs · 2019-01-04T20:47:35Z

TODO(me): still needs a rebase & addressing Rasto's review comment.

atiratree · 2019-01-07T10:18:46Z

This is probably OT, but could we have an extra build (script) without tests and validations? This would be useful in development when linking components with web-ui.

rawagner · 2019-01-07T10:50:53Z

This is probably OT, but could we have an extra build (script) without tests and validations? This would be useful in development when linking components with web-ui.

+1 i would find this useful too. It would be build step without prebuild.

rawagner · 2019-01-07T13:26:42Z

sass/components/CreateDiskRow/create-disk-row.scss

@@ -0,0 +1,3 @@
+.kubevirt-create-disk-row__action-buttons {


this was .createDiskRow__actionButtons button previously so the new style should look like .kubevirt-create-disk-row__action-buttons button or .kubevirt-create-disk-row__action-buttons .btn (i think the latter is better). I need to target nested buttons inside .kubevirt-create-disk-row__action-buttons

You are correct, I've missed that.

Since we need to target specific buttons, we can use .kubevirt-create-disk-row__action-button (singular) class name on <Button> components, if you agree.

We don't have to declare .kubevirt-create-disk-row__action-buttons (plural) if we don't need to style the enclosing <div> (button panel/container).

.kubevirt-create-disk-row__action-buttons button
.kubevirt-create-disk-row__action-buttons .btn

Nested CSS selectors should only be used when we don't have full control over the logical component's parts. In this case, we can simply use .kubevirt-create-disk-row__action-button, since PF-React's <Button> allows us to specify CSS class name.

.block tag selectors should be avoided if possible. CSS rules shouldn't be tied to HTML tags.

.block .nested-part selectors are possible, but unnecessary in this case.

vojtechszocs · 2019-01-07T14:30:30Z

This is probably OT, but could we have an extra build (script) without tests and validations? This would be useful in development when linking components with web-ui.

+1 i would find this useful too. It would be build step without prebuild.

Agreed, this can be useful. (Right now, you can temporarily change prebuild to e.g. x_prebuild in package.json scripts to get that behavior.)

openshift/console has a dev script that runs development build without pre-checks like linting or tests. It executes webpack in watch mode for better developer experience (DX).

In this project, we can use a dev script that runs Babel in watch mode with NODE_ENV=development, together with --copy-files to ensure that both JS & Sass output is up-to-date. I'll try to do this in a separate PR.

- use BEM convention with 'kubevirt' prefix - impose linting rules on import statements - update React component structure validation - update README & add docs

vojtechszocs · 2019-01-07T19:59:57Z

PR rebased & addressed all review comments.

Notable changes:

updated VmDetails.test.js.snap (vm-status-icon => kubevirt-vm-status__icon)
@rawagner moved the common dropdown-menu style to sass/components/Form/dropdown.scss
- had to use the sibling CSS combinator (+) since PF-React's <DropdownButton> renders two child components, <DropdownToggle> and <DropdownMenu>, but only applies user-provided CSS class to the first one
went over all Cosmos fixtures in the playground UI
- InlineEditRow: this component cannot be rendered standalone - it's an integral part of EditableDraggableTable, so the fixture throws error <tr> cannot appear as a child of <div>
  - solution: use empty-array fixture (no fixture objects), so Cosmos will skip it
- TableFactory: renders empty markup, which is confusing
  - solution: re-use rows/cols from EditableDraggableTable fixture and render something (something is always better than nothing)
- TemplateSource: tooltip is positioned at the top, so users can't see it
  - solution: add tooltipPlacement prop which defaults to top but use value bottom for the fixture

Also, the VmDetails fixture throws missing required prop error, we should fix that eventually.

Please treat Cosmos fixtures with same care as other code, as they demonstrate real-world usage of the given component and allow UX designers to review the current implementation.

rawagner · 2019-01-08T07:58:04Z

LGTM. action buttons in create disk row are broken https://github.com/kubevirt/web-ui-components/pull/150/files#r245651699 but lets fix it in a follow up #162

vojtechszocs requested review from atiratree, jelkosz, mareklibra, pcbailey and rawagner December 20, 2018 19:37

kubevirt-bot added the size/XXL label Dec 20, 2018

vojtechszocs force-pushed the style-restructure branch from f2f4ee4 to 4319568 Compare December 20, 2018 19:42

rawagner suggested changes Jan 3, 2019

View reviewed changes

atiratree reviewed Jan 3, 2019

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 4, 2019

rawagner reviewed Jan 7, 2019

View reviewed changes

Restructure styles, improve code and docs

52f90ee

- use BEM convention with 'kubevirt' prefix - impose linting rules on import statements - update React component structure validation - update README & add docs

vojtechszocs force-pushed the style-restructure branch from 3b4d02a to 52f90ee Compare January 7, 2019 19:38

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 7, 2019

rawagner approved these changes Jan 8, 2019

View reviewed changes

rawagner merged commit fa919cd into kubevirt:master Jan 8, 2019

This was referenced Jan 8, 2019

Add 'dev' script to do a development build #166

Merged

KubeVirt extension PoC openshift/console#888

Closed

Conversation

vojtechszocs commented Dec 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Styles

Code

Docs

Dependencies

Miscellaneous

Uh oh!

coveralls commented Dec 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 592

💛 - Coveralls

Uh oh!

vojtechszocs commented Dec 20, 2018

Uh oh!

rawagner left a comment

Choose a reason for hiding this comment

Uh oh!

rawagner Jan 3, 2019

Choose a reason for hiding this comment

Uh oh!

vojtechszocs Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rawagner Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

atiratree Jan 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vojtechszocs Jan 4, 2019

Choose a reason for hiding this comment

Uh oh!

atiratree Jan 3, 2019

Choose a reason for hiding this comment

Uh oh!

vojtechszocs Jan 4, 2019

Choose a reason for hiding this comment

Uh oh!

vojtechszocs commented Jan 4, 2019

Uh oh!

vojtechszocs commented Jan 4, 2019

Uh oh!

vojtechszocs commented Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atiratree commented Jan 7, 2019

Uh oh!

rawagner commented Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rawagner Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vojtechszocs Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

vojtechszocs commented Jan 7, 2019

Uh oh!

vojtechszocs commented Jan 7, 2019

Uh oh!

rawagner commented Jan 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vojtechszocs commented Dec 20, 2018 •

edited

Loading

coveralls commented Dec 20, 2018 •

edited

Loading

vojtechszocs Jan 4, 2019 •

edited

Loading

atiratree Jan 3, 2019 •

edited

Loading

vojtechszocs commented Jan 4, 2019 •

edited

Loading

rawagner commented Jan 7, 2019 •

edited

Loading

rawagner Jan 7, 2019 •

edited

Loading