ui: CSS modules for BarCharts components by koorosh · Pull Request #47484 · cockroachdb/cockroach

koorosh · 2020-04-14T17:26:17Z

Depends on: #47417
Depends on: cockroachdb/yarn-vendored#20
Related to: #47527

Current draft is just example for possible styles isolation with CSS modules.
It is required to make components self-contained and easy for extraction.

Before all css files were loaded into
global scope even if file was imported in
some particular module.

Now it is possible to define styles as before
and use old styles without changes, and as a
module, to do this - files has to be named as
someName.module.styl
In webpack config, style-loader is defined
two times with different file name matchers
to be able define style loaders with and without
modules.

barCharts component is a first candidate to try out css modules.

all styles related to barCharts are copied(!) to barCharts.module.styl file,
so in case another component somehow relies on styles defined for barCharts
it won't be affected.

Storybook is extended with stories related to barCharts

cockroach-teamcity · 2020-04-14T17:26:29Z

This change is

blathers-crl · 2020-04-14T17:54:53Z

❌ The GitHub CI (Cockroach) build has failed on eecf8b55.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

blathers-crl · 2020-04-15T15:02:18Z

❌ The GitHub CI (Cockroach) build has failed on 29120cab.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

nathanstilwell

I approve of this approach!

pkg/ui/src/views/statements/barCharts.module.styl

blathers-crl · 2020-04-23T14:37:19Z

❌ The GitHub CI (Cockroach) build has failed on 92aefcf5.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

dhartunian

Very exciting @koorosh!! Just a few notes on the visuals in the storybook.

Reviewed 5 of 8 files at r1, 2 of 8 files at r6, 1 of 1 files at r7, 5 of 5 files at r8.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh and @nathanstilwell)

pkg/ui/src/views/statements/barCharts.stories.tsx, line 18 at r8 (raw file):

const { statements } = statementsPagePropsFixture;

storiesOf("BarCharts", module)

All the charts stop getting more narrow as I squish the browser window horizontally. Is there something preventing them from getting very small in the storybook? They have to get quite small to fit into the table columns.

pkg/ui/src/views/statements/barCharts.stories.tsx, line 27 at r8 (raw file):

    return chartFactory(statements[0]);
  })
  .add("retryBarChart", () => {

The retry bar chart should render in red. Also it shows a negative number in my storybook. Can we render with positive?

pkg/ui/src/views/statements/statementsPage.fixture.ts, line 23 at r8 (raw file):

  "count": Long.fromNumber(3),
  "first_attempt_count": Long.fromNumber(100000),
  "max_retries": Long.fromNumber(10),

Not sure why but I end up seeing this:

dhartunian

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh and @nathanstilwell)

pkg/ui/src/views/statements/barCharts.tsx, line 141 at r8 (raw file):

      };

      const className = classNames(styles["bar-chart"], styles[`bar-chart-${type}`], {

The ${type} is what makes the bar chart red for retries so not sure what's missing...

koorosh · 2020-04-28T10:04:17Z

@dhartunian, thanks for your suggestions, indeed there some gaps with BarCharts which have to be fixed.

All the charts stop getting more narrow as I squish the browser window horizontally

Storybook had default min-size for the main panel. Removed this restriction so now Storybook more responsive.

Also to show bar charts within table columns - additional stories are added. Check out BarCharts > within column (width 150px). It represents charts within the same constraints as in the Statements table.

The retry bar chart should render in red. Also it shows a negative number in my storybook. Can we render with positive?

Fixed. The problem was with the wrong indentation for nested styles.

dhartunian

This all looks good from code perspective but causes a regression on the app right now. Could you take a look?

New:

Reviewed 1 of 7 files at r9, 1 of 1 files at r10, 6 of 7 files at r11, 4 of 4 files at r12.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh and @nathanstilwell)

koorosh · 2020-05-19T20:18:29Z

This all looks good from code perspective but causes a regression on the app right now. Could you take a look?

New:

Previous:

Reviewed 1 of 7 files at r9, 1 of 1 files at r10, 6 of 7 files at r11, 4 of 4 files at r12.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh and @nathanstilwell)

Regression with column width occurred because barCharts styles are overridden by outer component (statementsTable) and statementsTable is not yet work with CSS modules.
There is an example from statements.styl (refactored to CSS modules in another PR #48090)

.statements-table__col-rows, .statements-table__col-latency  // <-- defined as global CSS
  .bar-chart         // <-- trying to access class which is defined in CSS module
    min-width 150px

For me, it's a dilemma how to keep PR small enough with changes related directly to barCharts component and avoid such regressions.

Refactoring statements.styl to CSS modules is not an option because it will, in turn, require to refactor lots of modules that depend on it.

To test styles isolation for statements page we need storybook which displays entire Statements screen only. To make it work, RouterProvider decorator is added which connects router to dummy (empty) store. `statementsPage.fixture.ts` file contains snapshot of required props for StatementsPage component Release note: None

Before all css files were loaded into global scope even if file was imported in some particular module. Now it is possible to define styles as before and use old styles without changes, and as a module, to do this - files has to be named as someName.module.styl In webpack config, `style-loader` is defined two times with different file name matchers to be able define style loaders with and without modules. Release note: None

- added storybook with barCharts to show all possible types of barCharts - Also typescript doesn't recognize `module.styl` files as modules by default, so custom module declaration is added Release note: None

Previously, class names were constructed by simply accessing style modules class names and assigning it to classes. It was cumbersome and not readable at all. To enhance this, `classnames/bind` alternate is used, which allows simply put class names. Release note: None

Previously, Statements table altered styles of barCharts by overriding them. It was necessary to set fixed width for barCharts which were displayed in Statements table columns. When `barCharts` were refactored to use CSS modules it became impossible to override styles in such way (and it isn't react way). Now, it is possible to extend `barCharts` styles by providing additional `options` param with `classess` field. It consumes `root` and `label` clasess, and then applies them inside of component. Note, `statementsTable.module.styl` file has more styles than is used right now, because this entire file was ported here from another PR and this allows easily rebase it later. Release note: None

koorosh · 2020-05-20T11:58:42Z

@dhartunian , I managed to port changes related to custom styles for barCharts rendered in Statements table. It fixed the sizing of table columns with bar charts.
PS: It wasn't so big change as I thought before

dhartunian

Wonderful. Looks good to me. Let's ship it!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @koorosh, and @nathanstilwell)

dhartunian · 2020-05-20T15:47:05Z

bors r+

craig · 2020-05-20T16:34:48Z

Build failed

GitHub CI (Cockroach)

koorosh · 2020-05-27T14:45:17Z

bors r+

craig · 2020-05-27T15:55:37Z

Build succeeded

GitHub CI (Cockroach)

47513: ui: CSS modules for PlanView component r=koorosh a=koorosh Depends on: #47484 Related to: #47527 - Refactored font imports to correctly resolve paths when module is required from different locations; Fonts are imported directly from `app.styl` file which allows import `typography.styl` without dependencies. This change was required because importing `typography.styl` file from CSS modules failed with unresolved paths inside of `fonts.styl` file (which was required in `typography.styl` file). Before, ``` app.styl |-- typography.styl |-- fonts.styl ``` Now: ``` app.styl |-- typography.styl |-- fonts.styl ``` - Move all files related to PlanView component under `planView` directory - Added storybook for `PlanView` component - `planView.module.styl` file contains copy of styles (from `statements.styl`) which is used by component only. Release note: None 49779: opt: incorporate cast volatility r=RaduBerinde a=RaduBerinde This change incorporates the new cast volatility information into the VolatilitySet property. Release note: None 49783: geo/geotransform: implement ST_Transform r=sumeerbhola a=otan This PR implements ST_Transform, allowing the transformation from one SRID to another. The `geoprojbase` package defines a barebones set of types as well as a hardcoded list of SRIDs to keep in memory. I've only filled in a few for now, and will save updating this for a later PR. `geoproj` is strictly a C library interface library which performs the necessary transformations. `geotransform` is where the function is actually handled and to be used by `geo_builtins.go`. Resolves #49055 Resolves #49056 Resolves #49057 Resolves #49058 Release note (sql change): Implemented the ST_Transform function for geometries. 49804: opt: ON UPDATE cascades for Upsert r=RaduBerinde a=RaduBerinde This change implements ON UPDATE actions for Upsert operations. The existing machinery for Update can be used without modification. Release note: None Co-authored-by: Andrii Vorobiov <and.vorobiov@gmail.com> Co-authored-by: Radu Berinde <radu@cockroachlabs.com> Co-authored-by: Oliver Tan <otan@cockroachlabs.com>

koorosh mentioned this pull request Apr 15, 2020

ui: CSS modules for PlanView component #47513

Merged

koorosh added the do-not-merge bors won't merge a PR with this label. label Apr 15, 2020

koorosh force-pushed the koorosh/ui-statements-extraction--bar-charts-styles branch from eecf8b5 to 29120ca Compare April 15, 2020 14:33

nathanstilwell reviewed Apr 15, 2020

View reviewed changes

pkg/ui/src/views/statements/barCharts.module.styl Show resolved Hide resolved

koorosh marked this pull request as ready for review April 23, 2020 13:30

koorosh requested a review from a team April 23, 2020 13:30

koorosh force-pushed the koorosh/ui-statements-extraction--bar-charts-styles branch from 29120ca to 92aefcf Compare April 23, 2020 13:33

koorosh changed the title ~~ui: WIP. CSS modules for BarCharts components~~ ui: CSS modules for BarCharts components Apr 23, 2020

dhartunian mentioned this pull request Apr 23, 2020

ui: extract Statements Page components into standalone module #47527

Closed

23 tasks

koorosh force-pushed the koorosh/ui-statements-extraction--bar-charts-styles branch from 92aefcf to e953ca7 Compare April 24, 2020 07:41

dhartunian requested changes Apr 24, 2020

View reviewed changes

dhartunian reviewed Apr 24, 2020

View reviewed changes

koorosh force-pushed the koorosh/ui-statements-extraction--bar-charts-styles branch 3 times, most recently from 7697ae7 to 3077c47 Compare April 28, 2020 09:56

koorosh requested a review from dhartunian April 28, 2020 15:08

koorosh force-pushed the koorosh/ui-statements-extraction--bar-charts-styles branch from 3077c47 to 5a3d00c Compare May 4, 2020 14:25

koorosh removed the do-not-merge bors won't merge a PR with this label. label May 4, 2020

koorosh force-pushed the koorosh/ui-statements-extraction--bar-charts-styles branch 2 times, most recently from 5a4d023 to 3ab378b Compare May 19, 2020 13:11

dhartunian requested changes May 19, 2020

View reviewed changes

koorosh added 3 commits May 20, 2020 12:31

ui: CSS modules for barCharts component

fb71104

- added storybook with barCharts to show all possible types of barCharts - Also typescript doesn't recognize `module.styl` files as modules by default, so custom module declaration is added Release note: None

koorosh added 2 commits May 20, 2020 12:31

koorosh force-pushed the koorosh/ui-statements-extraction--bar-charts-styles branch from 3ab378b to 038b52f Compare May 20, 2020 11:50

koorosh requested a review from dhartunian May 20, 2020 11:59

dhartunian approved these changes May 20, 2020

View reviewed changes

craig bot merged commit 367dfbf into cockroachdb:master May 27, 2020

Conversation

koorosh commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Apr 14, 2020

Uh oh!

blathers-crl bot commented Apr 14, 2020

Uh oh!

blathers-crl bot commented Apr 15, 2020

Uh oh!

nathanstilwell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blathers-crl bot commented Apr 23, 2020

Uh oh!

dhartunian left a comment

Choose a reason for hiding this comment

Uh oh!

dhartunian left a comment

Choose a reason for hiding this comment

Uh oh!

koorosh commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhartunian left a comment

Choose a reason for hiding this comment

Uh oh!

koorosh commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

koorosh commented May 20, 2020

Uh oh!

dhartunian left a comment

Choose a reason for hiding this comment

Uh oh!

dhartunian commented May 20, 2020

Uh oh!

craig bot commented May 20, 2020

Build failed

Uh oh!

koorosh commented May 27, 2020

Uh oh!

craig bot commented May 27, 2020

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

koorosh commented Apr 14, 2020 •

edited

Loading

koorosh commented Apr 28, 2020 •

edited

Loading

koorosh commented May 19, 2020 •

edited

Loading