code-dot-org
Remove radium from DataBrowser and its dependencies
This is part of the work to stop using Radium. See https://github.com/code-dot-org/code-dot-org/pull/47367 for more discussion. This PR removes Radium from 19 components which collectively make up the bulk of App Lab's Data Browser:
- DataTable
- EditTableRow
- TableControls
- AddKeyRow
- DataBrowser
- EditTableListRow
- DataOverview
- ConfirmImportButton
- DataTableView
- EditLink
- ConfirmDeleteButton
- LibraryCategory
- EditKeyRow
- ColumnHeader
- DataLibraryPane
- KVPairs
- AddTableListRow
- AddTableRow
- DropdownField
Links
Google Sheet: Radium usage in apps/
Testing story
This work touches all parts of the Data Browser. A comparison video walks through the display and interaction with the components affected. There were no regressions found! Current Production as of 8/9:
https://user-images.githubusercontent.com/43474485/183724689-1e8e38fc-a1db-4795-9452-c8515820291d.mp4
Localhost as of https://github.com/code-dot-org/code-dot-org/pull/47527/commits/eb1d43b0f0879d459db48033615e2bd06d78a9f5 :
https://user-images.githubusercontent.com/43474485/183724849-1af795c6-1a01-40bb-a8e6-8c6f1645d18a.mp4
There are lots of separate commits here as I want to thoroughly find and test each component. In several cases, Radium could be removed easily. In other cases, scss modules were created in addition to removing Radium. Prior to committing these kinds of changes, I made some visually obvious changes to colors in the new modules to confirm that the module was actually determining the rules used. I looked through the DOM at each affected element to make sure that the expected rules (and if applicable, classes) were present and active.
Deployment strategy
Follow-up work
Several components here import style objects from https://github.com/code-dot-org/code-dot-org/blob/staging/apps/src/storage/dataBrowser/dataStyles.js As follow-up work we would like to convert this .js file into a scss module and then perform another round of updates on these components. (Slack thread for context)
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
A couple questions off the bat:
- Were the components where Radium was removed with no other changes (
AddKeyRow,AddTableListRow,AddTableRow, maybe more) just not using Radium at all? - It might not be worth it at this point since it seems like the changes are pretty straightforward, but would it have been possible to break this up a bit, or do all of these changes need to be made at once?
Maybe if in your list of components you could provide a little categorization of the components and the type of changes you had to make (eg, one category "doesn't actually use Radium", "had to create SCSS module", "just had to use spread operator instead of array syntax", etc)?
@bencodeorg The components where Radium was simply removed were not using it.
I went back and forth on the correct number of pull requests for these changes. I think it may have been an option to put out each separately, but it felt like it would be inefficient in terms of workflow. Some of these components depend on each other and/or interact closely on the website so it seemed simplest to test things together. I was also weary of having to write up 19 repetitive PRs, and requesting a lot of separate reviews. If any of these changes cause regressions with the Data Browser that I missed, I think I'd rather quickly revert the whole thing than have to immediately dig up exactly which component is causing the problem.
Happy to do things differently next time!
@bencodeorg The components where Radium was simply removed were not using it.
I went back and forth on the correct number of pull requests for these changes. I think it may have been an option to put out each separately, but it felt like it would be inefficient in terms of workflow. Some of these components depend on each other and/or interact closely on the website so it seemed simplest to test things together. I was also weary of having to write up 19 repetitive PRs, and requesting a lot of separate reviews. If any of these changes cause regressions with the Data Browser that I missed, I think I'd rather quickly revert the whole thing than have to immediately dig up exactly which component is causing the problem.
Happy to do things differently next time!
Oh definitely not suggesting 19 PRs, that would be a lot. I think just a bit of a guide about how a reviewer should think about looking at changes when there are this many files changed (eg, a categorization of the types of changes made) can be helpful.
@madelynkasula Please find your requested changes in this newest commit: https://github.com/code-dot-org/code-dot-org/pull/47527/commits/42fe57b0d3d634281079fff8864bb345a902b18d