Remove Radium from Animation Picker components #47478

mikeharv · 2022-08-03T21:11:32Z

This is part of the work to stop using Radium. See #47367 for more discussion. Deprecate Radium from the Animation Picker components.

The techniques used in this work largely resemble what was done for other Radium components.
However, this feature relies on the class(es) associated with a target animation to determine if it should be included in the filtered results displayed when changing categories. Because we are now using a SCSS module, all targets have multiple classes. To address this, we can ignore all classes except the first when determining if it's a category match. (File context and Slack thread)

Links

Google Sheet: Radium usage in apps/

Testing story

Tested across Sprite Lab, Game Lab and the "Sprite Upload" feature. No regression found when comparing against production (or levelbuilder for Sprite Upload).

Deployment strategy

Follow-up work

Jira: STAR-2375: Stop using classes to determine animation categories in Animation Picker

Privacy

Security

Caching

PR Checklist:

Tests provide adequate coverage
Privacy and Security impacts have been assessed
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

mikeharv · 2022-08-22T15:38:15Z

apps/src/p5lab/AnimationPicker/AnimationPickerBody.jsx

@@ -127,7 +127,7 @@ export default class AnimationPickerBody extends React.Component {
  };

  onCategoryChange = event => {
-    const categoryQuery = event.target.className;
+    const categoryQuery = event.target.className.split(' ')[0];


This feature relies on the class(es) associated with a target animation to determine if it should be included in the filtered results displayed when changing categories. Because we are now using a SCSS module, all targets have multiple classes. These SCSS classes are always added after the class representing an animation's category.

Update: We refactored this to use a data-* attribute instead of a class.

maddiedierker · 2022-08-22T17:30:45Z

apps/src/p5lab/AnimationPicker/AnimationPickerBody.jsx

@@ -199,7 +201,7 @@ export default class AnimationPickerBody extends React.Component {
    if (!this.props.libraryManifest) {
      return <div>{msg.loading()}</div>;
    }
-    const {searchQuery, categoryQuery, results} = this.state;
+    let {searchQuery, categoryQuery, results} = this.state;


this is probably leftover from previous changes, but this should probably stay as const because we shouldn't be mutating anything in this.state

Great catch! This was leftover from when I had tried splitting the string at this point.

maddiedierker · 2022-08-22T17:33:45Z

apps/src/p5lab/AnimationPicker/AnimationPickerListItem.jsx

+            hover && style.multiSelectBorder,
+            hover && style.hoverBorder,
+            selected && style.selectBorder,


now that we're using CSS, i wonder if this state is necessary anymore or if we can replace it with :hover and :active styles

I actually reverted this today because it seemed to not be working as expected. Happy to take another look.

eh, it's also an improvement we can make later. if it isn't easily fixable, i'd say it's outside the scope of this refactor since we can get rid of radium without this additional change

maddiedierker · 2022-08-22T17:35:07Z

apps/src/p5lab/AnimationPicker/animation-picker-body.module.scss

+@import 'color.scss';
+
+.allAnimations {
+    color: $purple;


nit: the indentation from this line on is one tab too far indented

These kinds of things (indentation and newline) can be caught if we disable prettier ... or re-add these rules in. Do you have a thought about best practice here?

ahh, i should've checked the stylelint.config.js file -- now i see "rule-empty-line-before" and "declaration-empty-line-before" are disabled. do you know which rule enforces indentation style?

I think it's here https://github.com/stylelint/stylelint-config-standard/blob/main/index.js#L96 but I'm now unable to make any warnings appear ...

Oh, I was running the command with --fix so the warnings weren't showing up 🤣 . But yeah, if I disable prettier by deleting it in our config, and then run npx stylelint "**/*.scss, then I get warnings like

src/lib/ui/Pagination.scss 3:5 ✖ Expected indentation of 2 spaces indentation 4:5 ✖ Expected indentation of 2 spaces indentation 5:5 ✖ Expected indentation of 2 spaces indentation 7:5 ✖ Expected indentation of 2 spaces indentation

This originally came up with Ben in a PR I did recently #47716 (comment)

weeeeeeeeird, why does prettier disable that rule? for now, let's just manually fix this and i'll do a pass with the linting rules and make a PR to fix all the existing violations

or maybe we should just remove the prettier plugin... the rules it's disabling/conflicting with are ones i feel like we want to keep 🙃

I started this branch and can't remember where I left off ... but its goal is to remove prettier. It was fairly straightforward I believe https://github.com/code-dot-org/code-dot-org/tree/remove-prettier

I think I just got sidetracked with other work ... I'll make a PR for it shortly

maddiedierker · 2022-08-22T17:35:40Z

apps/src/p5lab/AnimationPicker/animation-picker-body.module.scss

+    font-family: 'Gotham 7r', sans-serif;
+    cursor: pointer
+  }
+  .breadCrumbs {


nit: add an empty newline between classes

maddiedierker · 2022-08-22T17:36:33Z

apps/src/p5lab/AnimationPicker/animation-picker-body.module.scss

+  .pagination {
+    float: right;
+    display: inline;
+    margin-top: 10


nit: a couple of ; missing in this file -- i think all of my nits should be addressed by our new linting, which was unfortunately added after you started this branch. if you merge staging into this branch, try making a change to this file and make sure the linting catches these rules for us!

Interesting. I actually merged staging into this branch about four ago and have since made changes.

maddiedierker · 2022-08-22T17:37:23Z

apps/src/p5lab/AnimationPicker/animation-picker-list-item.module.scss

+$hover-plus-size: 24px;
+
+.root {
+    float: left;


nit: styles are indented 1 tab too far (same thing about linting -- make a single change and see if our linting catches this so we don't have to do it manually anymore!)

mikeharv · 2022-08-22T21:00:22Z

@megcrenshaw @madelynkasula PTAL. I had to do all of the linting manually? I do have recent staging merged, but I'm not sure how to get my setup to detect/fix these indentation problems automatically.

megcrenshaw · 2022-08-22T21:35:33Z

@mikeharv

@megcrenshaw @madelynkasula PTAL. I had to do all of the linting manually? I do have recent staging merged, but I'm not sure how to get my setup to detect/fix these indentation problems automatically.

I think you will need to remove prettier from the config, as in https://github.com/code-dot-org/code-dot-org/blob/ca5971318f23f884bf101f7cbb85afb8cac5651c/apps/stylelint.config.js#L2, and then run npx stylelint "**/*.scss" --fix from the apps directory ... But I'm currently working on a PR https://github.com/code-dot-org/code-dot-org/pull/47758/files to remove this so that you won't need to do this manually. That's the hope ...

maddiedierker

🎉 🎉 🎉

megcrenshaw · 2022-08-22T21:55:49Z

apps/src/p5lab/AnimationPicker/AnimationPickerBody.jsx

@@ -127,7 +129,7 @@ export default class AnimationPickerBody extends React.Component {
  };

  onCategoryChange = event => {
-    const categoryQuery = event.target.className;
+    const categoryQuery = getCategory(event.target);


This is lovely!

megcrenshaw · 2022-08-22T21:56:49Z

apps/src/p5lab/AnimationPicker/AnimationPickerBody.jsx

@@ -300,40 +301,11 @@ export default class AnimationPickerBody extends React.Component {
  }
 }

-export const UnconnectedAnimationPickerBody = Radium(AnimationPickerBody);
+export const UnconnectedAnimationPickerBody = AnimationPickerBody;


I don't think you need this now

Although that will require a line change in the AnimationPickerBodyTest file, but I think that's okay too

@megcrenshaw I'm actually unclear on how this thing that gets imported is different from the default export above. (Maybe that's what you're getting at though - they're the same?)

If that's the case, what was the original value of having a default export (without Radium) and this separate export (with Radium) for the test? Was this component not actually using Radium to begin with?

Wow, you're right, this is bizarre.

Usually, we export UnconnectedComponentName which isn't hooked up to Radium and/or state, and we use that for testing. And then we default export ComponentName with Radium and/or state for other components to consume.

So this is weird because the original code for UnconnectedAnimationPickerBody is actually connected to Radium, AND the default export was actually not connected to Radium, AND the component-with-Radium was the one being tested 🤯.

So you're right, it does seem that the components consuming this component were not using Radium. And you're right that at this point the two are the same. More reason to just get rid of this confusion!

Updated: 52921b5

megcrenshaw · 2022-08-22T21:59:31Z

apps/src/p5lab/AnimationPicker/AnimationPickerListItem.jsx

 import {PlayBehavior} from '../constants';
 import * as shapes from '../shapes';
 import AnimationPreview from './AnimationPreview';
+import style from './animation-picker-list-item.module.scss';
+
+import classNames from 'classnames';

 const THUMBNAIL_SIZE = 105;


I think you can remove this line and the one below it too?

@megcrenshaw These constants are still used below at L76-77 to pass the height and width for the AnimationPreview component.
These values are set here and within the module, which doesn't seem great now that you mention it. I wonder how we can have one source of truth for thumbnail size across both components.

Ah, okay, I missed that. Maddie mentioned this syntax in this thread:

:export { myCssVariable: $my-css-variable; }

Would that work here?

Ugh. Just remembered that $thumbnail-size: 105px; while const THUMBNAIL_SIZE = 105;
I can't just export the pixel value because the other component that also uses the values (AnimationPreview) is expecting a number for these properties.

@megcrenshaw (cc @madelynkasula)
This worked (0e0d782), but it did hit linting errors. I ended up doing a git commit --no-verify to get around this, but I'd like to confirm that this is okay before merging something that might just be caught later in the pipeline any way.

megcrenshaw

Two small nits, but this looks great! Nice work

maddiedierker · 2022-08-23T21:53:54Z

this looks good to merge! great work, mike!

megcrenshaw · 2022-08-24T02:28:55Z

Great work, Mike! Lots of good stuff in here 🥳

mikeharv added 2 commits August 3, 2022 17:10

create module

059316c

use scss module and deprecate radium

0366583

mikeharv changed the title ~~Mike/animation picker list item module~~ animation picker list item module Aug 3, 2022

mikeharv added 7 commits August 4, 2022 17:10

create style module for animation picker body

d6d2fbf

deprecate radium

ce2d8f7

Merge branch 'staging' into mike/animation-picker-list-item-module

6db1042

rename variables, remove px from 0 values, use calc()

6194766

remove unneeded calc

6cd617a

revert subclasses

648fbea

only use first class to populate results

59a6101

mikeharv commented Aug 22, 2022

View reviewed changes

mikeharv marked this pull request as ready for review August 22, 2022 15:48

mikeharv requested a review from a team August 22, 2022 15:49

use data-category attribute instead of category as class

5398916

maddiedierker reviewed Aug 22, 2022

View reviewed changes

mikeharv changed the title ~~animation picker list item module~~ Remove Radium from Animation Picker Aug 22, 2022

mikeharv changed the title ~~Remove Radium from Animation Picker~~ Remove Radium from Animation Picker components Aug 22, 2022

mikeharv added 5 commits August 22, 2022 14:22

Merge branch 'staging' into mike/animation-picker-list-item-module

72f6dc0

fix indentation and spacing

192cfb9

add missing semicolons

72c1513

adjust indentation

0aaefa0

use const for values that should not change

37a8732

maddiedierker approved these changes Aug 22, 2022

View reviewed changes

megcrenshaw reviewed Aug 22, 2022

View reviewed changes

megcrenshaw approved these changes Aug 22, 2022

View reviewed changes

simplify exports

52921b5

mikeharv added 10 commits August 23, 2022 10:31

move calculations to module

0e0d782

updating linting rules to allow variable exports

e66b126

fix rule

787df70

add period

1b161c7

remove prettier

9664723

ignore camelCase for exported variable labels

c0ea82e

Update stylelint.config.js

55c90a0

Update animation-picker-list-item.module.scss

62e8d04

use double quotes

0e6161b

Merge branch 'staging' into mike/animation-picker-list-item-module

5cf08f5

mikeharv merged commit 5fdf203 into staging Aug 23, 2022

mikeharv deleted the mike/animation-picker-list-item-module branch August 23, 2022 22:27

mikeharv restored the mike/animation-picker-list-item-module branch August 24, 2022 12:35

mikeharv deleted the mike/animation-picker-list-item-module branch August 24, 2022 12:37

This was referenced Aug 24, 2022

Revert "Remove Radium from Animation Picker components" #47795

Closed

Animation Library - Cast string to number after import #47878

Merged

Remove Radium from Animation Picker components #47478

Remove Radium from Animation Picker components #47478

Conversation

mikeharv commented Aug 3, 2022 • edited

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

megcrenshaw Aug 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

megcrenshaw Aug 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeharv commented Aug 22, 2022

megcrenshaw commented Aug 22, 2022 • edited by mikeharv

maddiedierker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

megcrenshaw Aug 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeharv Aug 23, 2022 • edited

Choose a reason for hiding this comment

megcrenshaw left a comment

Choose a reason for hiding this comment

maddiedierker commented Aug 23, 2022

megcrenshaw commented Aug 24, 2022

mikeharv commented Aug 3, 2022 •

edited

megcrenshaw Aug 22, 2022 •

edited

megcrenshaw Aug 22, 2022 •

edited

megcrenshaw commented Aug 22, 2022 •

edited by mikeharv

megcrenshaw Aug 22, 2022 •

edited

mikeharv Aug 23, 2022 •

edited