Skip to content

Conversation

@michaelchadwick
Copy link
Contributor

@michaelchadwick michaelchadwick commented Nov 19, 2025

Refs ilios/ilios#6608
Fixes ilios/ilios#6669

After a suggestion, this has been renamed from ilios-browser-defaults to normalize-external-editor to better reflect its utility (resetting styles for HtmlEditor component instances).

@michaelchadwick michaelchadwick force-pushed the 6669-replace-ilios-browser-defaults-sass-mixin branch from e894e95 to 83ec515 Compare November 19, 2025 21:59
@netlify
Copy link

netlify bot commented Nov 19, 2025

Deploy Preview for ilios-frontend ready!

Name Link
🔨 Latest commit e894e95
🔍 Latest deploy log https://app.netlify.com/projects/ilios-frontend/deploys/691e3db1db3af6000767d694
😎 Deploy Preview https://deploy-preview-8961--ilios-frontend.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify
Copy link

netlify bot commented Nov 19, 2025

Deploy Preview for ilios-frontend ready!

Name Link
🔨 Latest commit 03a3525
🔍 Latest deploy log https://app.netlify.com/projects/ilios-frontend/deploys/6920d7feadbced0008cc1d8e
😎 Deploy Preview https://deploy-preview-8961--ilios-frontend.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link
Member

@jrjohnson jrjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a mixin this name was ok and worked, but as a class I think it probably needs to be split up into reset-list and reset-paragraph (or something like that) and turned into separate classes. class="sessiondescription ilios-browser-defaults" is confusing.

@michaelchadwick
Copy link
Contributor Author

As a mixin this name was ok and worked, but as a class I think it probably needs to be split up into reset-list and reset-paragraph (or something like that) and turned into separate classes.

I can understand wanting to rename it, but why separate classes? I mean, unless you think we need a utility class that just sets p { margin: 0; }. The list and paragraph rules seem to fit into a normalize-external-editor grouping that has always gone together and would continue to do that.

@jrjohnson
Copy link
Member

I like that name. I was only pulling them apart because I couldn't think of how to keep them together with a good name. Go for it.

@michaelchadwick michaelchadwick force-pushed the 6669-replace-ilios-browser-defaults-sass-mixin branch from 83ec515 to 7649c9e Compare November 21, 2025 16:14
@jrjohnson jrjohnson removed the request for review from stopfstedt November 21, 2025 17:30
@jrjohnson jrjohnson added the run ui tests Run the expensive UI tests label Nov 21, 2025
@michaelchadwick michaelchadwick force-pushed the 6669-replace-ilios-browser-defaults-sass-mixin branch from 7649c9e to 03a3525 Compare November 21, 2025 21:22
Copy link
Member

@dartajax dartajax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems fine to me - not sure what to test but clicked around quite a bit and looked for anything awry - nothing found - approval granted

@dartajax dartajax merged commit f462107 into ilios:master Nov 21, 2025
32 checks passed
@michaelchadwick michaelchadwick deleted the 6669-replace-ilios-browser-defaults-sass-mixin branch December 4, 2025 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run ui tests Run the expensive UI tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ilios-browser-defaults

3 participants