Skip to content

Commit

Permalink
[Storybook] Fix styles in editor stories (#802)
Browse files Browse the repository at this point in the history
## Summary:

After upgrading to Storybook v7 (#790), Nicole noticed that some of our styles in Editor stories was missing/broken. 

This PR fixes it with the following changes (not all required, but the move for `main.ts` => `main.ts` seemed really easy and very useful!):

  * 🛠️ Add explicit deps for less-loader, css-loader, and style-loader. Previously, we depended on these modules indirectly and didn't have explicit control over which version of each we had. Turns out we had some really old versions and upgrading them fixed some of the issues.
  * ✨ Migrate Storybook config to TS. 
  * ✨ Use Storybook config types and fix up framework ref
  * 🛠️ Make less-loader work as it used to (eager math evaluation)
  * 🛠️ Ensure perseus-editor.less is imported in Editor demo stories

<img width="800" alt="image" src="https://github.com/Khan/perseus/assets/77138/16c35a72-faf6-4cca-a007-18910ea39444">

## Dependency Details

I've included `yarn why` output for both `style-loader` and `css-loader` modules, which are part of this PR (for historical purposes). Both of these were "anchored" to a super-old version because of the `size-limit` dependency in the `simple-markdown` package. We don't use or need that package anymore so I've removed it. I think it's safe to upgrade them as we don't use them to build our production bundles (we use Rollup for that and not Webpack!).

<details>
<summary>
<code>style-loader</code>
</summary>

```sh
$ yarn why style-loader
yarn why v1.22.19
[1/4] 🤔  Why do we have the module "style-loader"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "[email protected]"
info Reasons this module exists
   - "_project_#@khanacademy#simple-markdown#size-limit" depends on it
   - Hoisted from "_project_#@khanacademy#simple-markdown#size-limit#style-loader"
info Disk size without dependencies: "404KB"
info Disk size with unique dependencies: "756KB"
info Disk size with transitive dependencies: "3.39MB"
info Number of shared dependencies: 12
=> Found "@storybook/builder-webpack5#[email protected]"
info This module exists because "_project_#@storybook#react-webpack5#@storybook#builder-webpack5" depends on it.
info Disk size without dependencies: "132KB"
info Disk size with unique dependencies: "132KB"
info Disk size with transitive dependencies: "132KB"
info Number of shared dependencies: 0
✨  Done in 0.48s.
```

</details>

<details>
<summary>
<code>css-loader</code>
</summary>

```sh
$ yarn why css-loader
yarn why v1.22.19
[1/4] 🤔  Why do we have the module "css-loader"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "[email protected]"
info Reasons this module exists
   - "_project_#@khanacademy#simple-markdown#size-limit" depends on it
   - Hoisted from "_project_#@khanacademy#simple-markdown#size-limit#css-loader"
info Disk size without dependencies: "700KB"
info Disk size with unique dependencies: "3.31MB"
info Disk size with transitive dependencies: "7.15MB"
info Number of shared dependencies: 26
=> Found "@storybook/builder-webpack5#[email protected]"
info This module exists because "_project_#@storybook#react-webpack5#@storybook#builder-webpack5" depends on it.
✨  Done in 0.51s.
```

</details>

Issue: LC-1459

## Test plan:

Run `yarn; yarn start`
Navigate to **Perseus** >> **Editor** >> **Demo** and compare to the Jira ticket's "Before" screenshot. It should match!

Author: jeremywiebe

Reviewers: jeremywiebe, nedredmond, handeyeco, SonicScrewdriver

Required Reviewers:

Approved By: nedredmond

Checks: ⌛ gerald, ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage, ⏭  Publish npm snapshot, ✅ Publish Storybook to Chromatic (ubuntu-latest, 16.x), ✅ Extract i18n strings (ubuntu-latest, 16.x), ✅ Check builds for changes in size (ubuntu-latest, 16.x), ✅ Cypress (ubuntu-latest, 16.x), ✅ Jest Coverage (ubuntu-latest, 16.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 16.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 16.x), ✅ gerald

Pull Request URL: #802
  • Loading branch information
jeremywiebe authored Nov 15, 2023
1 parent 9b28bbc commit 57e0e18
Show file tree
Hide file tree
Showing 11 changed files with 236 additions and 2,090 deletions.
5 changes: 5 additions & 0 deletions .changeset/curvy-pumpkins-bow.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/simple-markdown": minor
---

Remove unused dependencies: size-limit and uglify-js
5 changes: 5 additions & 0 deletions .changeset/grumpy-cameras-fetch.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/simple-markdown": minor
---

Remove unused dependencies: size-limit and uglify-js
5 changes: 5 additions & 0 deletions .changeset/large-mails-march.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"perseus-build-settings": minor
---

Adjust builds so that less handles math the way it did in older versions.
26 changes: 13 additions & 13 deletions .storybook/main.js → .storybook/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ const path = require("path");
const fs = require("fs");
const glob = require("fast-glob");

module.exports = {
import type {StorybookConfig} from "@storybook/react-webpack5";

const config: StorybookConfig = {
framework: "@storybook/react-webpack5",

stories: [
// NOTE(jeremy): This glob is extremely finicky! I would have written
// this as a negated match to exclude node_modules, but I was never
Expand All @@ -19,9 +23,9 @@ module.exports = {
],

addons: [
getAbsolutePath("@storybook/addon-links"),
getAbsolutePath("@storybook/addon-essentials"),
getAbsolutePath("@storybook/addon-a11y"),
"@storybook/addon-links",
"@storybook/addon-essentials",
"@storybook/addon-a11y",
],

// NOTE(kevinb): We customize the padding a bit so that so that stories
Expand Down Expand Up @@ -93,7 +97,10 @@ module.exports = {
loader: "css-loader",
options: {url: false},
},
"less-loader",
{
loader: "less-loader",
options: {lessOptions: {math: "always"}},
},
],
},
{
Expand Down Expand Up @@ -121,16 +128,9 @@ module.exports = {
return updateWebpackConfig;
},

framework: {
name: getAbsolutePath("@storybook/react-webpack5"),
options: {},
},

docs: {
autodocs: true,
},
};

function getAbsolutePath(value) {
return dirname(require.resolve(join(value, "package.json")));
}
module.exports = config;
64 changes: 64 additions & 0 deletions 6881044863.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
{
"changesets": [
{
"releases": [
{
"name": "@khanacademy/simple-markdown",
"type": "minor"
}
],
"summary": "Remove unused dependencies: size-limit and uglify-js",
"id": "curvy-pumpkins-bow"
},
{
"releases": [
{
"name": "@khanacademy/simple-markdown",
"type": "minor"
}
],
"summary": "Remove unused dependencies: size-limit and uglify-js",
"id": "grumpy-cameras-fetch"
}
],
"releases": [
{
"name": "@khanacademy/simple-markdown",
"type": "minor",
"oldVersion": "0.10.4",
"changesets": [
"curvy-pumpkins-bow",
"grumpy-cameras-fetch"
],
"newVersion": "0.11.0"
},
{
"name": "@khanacademy/perseus",
"type": "patch",
"oldVersion": "13.1.0",
"changesets": [],
"newVersion": "13.1.1"
},
{
"name": "@khanacademy/pure-markdown",
"type": "patch",
"oldVersion": "0.2.10",
"changesets": [],
"newVersion": "0.2.11"
},
{
"name": "@khanacademy/perseus-editor",
"type": "patch",
"oldVersion": "2.9.1",
"changesets": [],
"newVersion": "2.9.2"
},
{
"name": "@khanacademy/perseus-linter",
"type": "none",
"oldVersion": "0.3.8",
"changesets": [],
"newVersion": "0.3.8"
}
]
}
3 changes: 3 additions & 0 deletions config/build/rollup.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,9 @@ const createConfig = (
// into our libraries when our consumers should be the ones
// handling asset bundling.
url: false,
less: {
math: "always",
},
}),
babel({
babelHelpers:
Expand Down
5 changes: 4 additions & 1 deletion config/cypress/cypress.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,10 @@ module.exports = defineConfig({
},
},
},
"less-loader",
{
loader: "less-loader",
options: {lessOptions: {math: "always"}},
},
],
},
],
Expand Down
6 changes: 4 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
"babel-loader": "^9.1.3",
"babel-plugin-istanbul": "^6.1.1",
"cross-env": "^5.2.0",
"css-loader": "^6.8.1",
"cypress": "^12.5.1",
"cypress-jest-adapter": "^0.1.1",
"cypress-wait-until": "^1.7.2",
Expand Down Expand Up @@ -95,8 +96,8 @@
"jest-extended": "^2.0.0",
"jest-serializer-html": "^7.1.0",
"jest-specific-snapshot": "^5.0.0",
"less": "~3.0.0",
"less-loader": "^4",
"less": "^4.2.0",
"less-loader": "^11.1.3",
"nyc": "^15.1.0",
"prettier": "^2.6.2",
"react-json-view": "^1.19.1",
Expand All @@ -114,6 +115,7 @@
"rollup-plugin-styles": "^4.0.0",
"sloc": "^0.2.1",
"storybook": "7.5.3",
"style-loader": "^3.3.3",
"typescript": "4.9.5",
"typescript-coverage-report": "^0.7.0",
"webpack": "5",
Expand Down
2 changes: 2 additions & 0 deletions packages/perseus-editor/src/__stories__/editor.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import {registerAllWidgetsAndEditorsForTesting} from "../util/register-all-widge

import type {PerseusRenderer} from "@khanacademy/perseus";

import "../styles/perseus-editor.less";

registerAllWidgetsAndEditorsForTesting(); // SIDE_EFFECTY!!!! :cry:

export default {
Expand Down
4 changes: 1 addition & 3 deletions packages/simple-markdown/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,7 @@
"devDependencies": {
"@types/react-dom": ">=16.0.0",
"perseus-build-settings": "^0.2.1",
"size-limit": "^0.21.1",
"typescript": "^3.6.4",
"uglify-js": "^3.6.7"
"typescript": "^3.6.4"
},
"peerDependencies": {
"react": "16.14.0",
Expand Down
Loading

0 comments on commit 57e0e18

Please sign in to comment.