From 4f57b454baa100f8856f8cd17621912abbb1bf87 Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Fri, 6 Mar 2026 13:41:07 -0800 Subject: [PATCH 1/6] Prevent Negative Margins Adding a linting rule which will prevent negative margins. These are a problem in Ilios because they allow a component to exist outside of it's intended layout which makes working with multiple components in a grouping difficult. --- packages/frontend/.stylelintrc.js | 3 +++ packages/ilios-common/.stylelintrc.js | 3 +++ packages/lti-course-manager/.stylelintrc.js | 3 +++ packages/lti-dashboard/.stylelintrc.js | 3 +++ packages/test-app/.stylelintrc.js | 3 +++ 5 files changed, 15 insertions(+) diff --git a/packages/frontend/.stylelintrc.js b/packages/frontend/.stylelintrc.js index 1d5417c4f5..2d3764fb29 100644 --- a/packages/frontend/.stylelintrc.js +++ b/packages/frontend/.stylelintrc.js @@ -4,6 +4,9 @@ module.exports = { plugins: ['stylelint-scales'], extends: ['stylelint-config-recommended-scss'], rules: { + 'declaration-property-value-disallowed-list': { + '/^margin/': ['/-[\\d.]/'], + }, 'property-disallowed-list': ['font-size', 'line-height'], 'font-weight-notation': 'numeric', 'scales/font-weights': [200, 400, 600], diff --git a/packages/ilios-common/.stylelintrc.js b/packages/ilios-common/.stylelintrc.js index 1d5417c4f5..2d3764fb29 100644 --- a/packages/ilios-common/.stylelintrc.js +++ b/packages/ilios-common/.stylelintrc.js @@ -4,6 +4,9 @@ module.exports = { plugins: ['stylelint-scales'], extends: ['stylelint-config-recommended-scss'], rules: { + 'declaration-property-value-disallowed-list': { + '/^margin/': ['/-[\\d.]/'], + }, 'property-disallowed-list': ['font-size', 'line-height'], 'font-weight-notation': 'numeric', 'scales/font-weights': [200, 400, 600], diff --git a/packages/lti-course-manager/.stylelintrc.js b/packages/lti-course-manager/.stylelintrc.js index 1d5417c4f5..2d3764fb29 100644 --- a/packages/lti-course-manager/.stylelintrc.js +++ b/packages/lti-course-manager/.stylelintrc.js @@ -4,6 +4,9 @@ module.exports = { plugins: ['stylelint-scales'], extends: ['stylelint-config-recommended-scss'], rules: { + 'declaration-property-value-disallowed-list': { + '/^margin/': ['/-[\\d.]/'], + }, 'property-disallowed-list': ['font-size', 'line-height'], 'font-weight-notation': 'numeric', 'scales/font-weights': [200, 400, 600], diff --git a/packages/lti-dashboard/.stylelintrc.js b/packages/lti-dashboard/.stylelintrc.js index 1d5417c4f5..2d3764fb29 100644 --- a/packages/lti-dashboard/.stylelintrc.js +++ b/packages/lti-dashboard/.stylelintrc.js @@ -4,6 +4,9 @@ module.exports = { plugins: ['stylelint-scales'], extends: ['stylelint-config-recommended-scss'], rules: { + 'declaration-property-value-disallowed-list': { + '/^margin/': ['/-[\\d.]/'], + }, 'property-disallowed-list': ['font-size', 'line-height'], 'font-weight-notation': 'numeric', 'scales/font-weights': [200, 400, 600], diff --git a/packages/test-app/.stylelintrc.js b/packages/test-app/.stylelintrc.js index 1d5417c4f5..2d3764fb29 100644 --- a/packages/test-app/.stylelintrc.js +++ b/packages/test-app/.stylelintrc.js @@ -4,6 +4,9 @@ module.exports = { plugins: ['stylelint-scales'], extends: ['stylelint-config-recommended-scss'], rules: { + 'declaration-property-value-disallowed-list': { + '/^margin/': ['/-[\\d.]/'], + }, 'property-disallowed-list': ['font-size', 'line-height'], 'font-weight-notation': 'numeric', 'scales/font-weights': [200, 400, 600], From 7488a6e16d3a1e67cd4ec99323628688b20d7e20 Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Fri, 6 Mar 2026 13:42:56 -0800 Subject: [PATCH 2/6] Remove Negative Margins from New Subject Search I think these made the layout worse, gone now. I also added a number of screenshot tests and a coruse search test as I was checking this. --- .../components/reports/new-subject.scss | 1 - .../components/reports/new-subject-test.gjs | 34 ++++++++++++++++++- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/packages/frontend/app/styles/components/reports/new-subject.scss b/packages/frontend/app/styles/components/reports/new-subject.scss index 8bbc347361..d1285a1b5b 100644 --- a/packages/frontend/app/styles/components/reports/new-subject.scss +++ b/packages/frontend/app/styles/components/reports/new-subject.scss @@ -67,7 +67,6 @@ .results { @include m.ilios-list-reset; - margin-top: -0.25rem; grid-column: 2; background: var(--white); border-width: 0 1px 1px 1px; diff --git a/packages/frontend/tests/integration/components/reports/new-subject-test.gjs b/packages/frontend/tests/integration/components/reports/new-subject-test.gjs index 85639efdd3..e1b4971ba3 100644 --- a/packages/frontend/tests/integration/components/reports/new-subject-test.gjs +++ b/packages/frontend/tests/integration/components/reports/new-subject-test.gjs @@ -1,6 +1,6 @@ import Service from '@ember/service'; import { module, test } from 'qunit'; -import { setupRenderingTest } from 'frontend/tests/helpers'; +import { setupRenderingTest, takeComponentScreenshot } from 'frontend/tests/helpers'; import { render } from '@ember/test-helpers'; import { setupMirage } from 'frontend/tests/test-support/mirage'; import { component } from 'frontend/tests/pages/components/reports/new-subject'; @@ -70,6 +70,7 @@ module('Integration | Component | reports/new-subject', function (hooks) { 'subject dropdown value is correct', ); await component.subjects.choose(subjectVal); + await takeComponentScreenshot(assert); await component.objects.choose('null'); if (!exceptedSubjects.includes(subjectVal)) { assert.strictEqual(component.objects.items[0].value, '', '"Anything" is first object option'); @@ -111,6 +112,7 @@ module('Integration | Component | reports/new-subject', function (hooks) { this.owner.register('service:current-user', CurrentUserMock); await render(); + await takeComponentScreenshot(assert); assert.strictEqual(component.componentTitle, 'New Report'); assert.strictEqual(component.schools.items.length, 4); @@ -196,7 +198,9 @@ module('Integration | Component | reports/new-subject', function (hooks) { await component.objects.choose('mesh term'); assert.notOk(component.meshTerm.hasSelectedTerm, 'no mesh term selected'); await component.meshTerm.meshManager.search.set('descriptor 0'); + await takeComponentScreenshot(assert, 'with descriptor'); await component.meshTerm.meshManager.searchResults[0].add(); + await takeComponentScreenshot(assert, 'with term selected'); assert.strictEqual( component.meshTerm.selectedTerm, 'descriptor 0', @@ -241,7 +245,9 @@ module('Integration | Component | reports/new-subject', function (hooks) { await component.objects.choose('instructor'); assert.notOk(component.instructor.hasSelectedInstructor); await component.instructor.userSearch.searchBox.set('Rusty'); + await takeComponentScreenshot(assert, 'with instructor'); await component.instructor.userSearch.results.items[0].click(); + await takeComponentScreenshot(assert, 'with instructor selected'); assert.strictEqual(component.instructor.selectedInstructor, 'Rusty M. Mc0son'); await component.instructor.removeSelectedInstructor(); assert.notOk(component.instructor.hasSelectedInstructor); @@ -413,10 +419,35 @@ module('Integration | Component | reports/new-subject', function (hooks) { await component.objects.choose('instructor'); assert.ok(component.instructor.userSearch.isVisible); await component.instructor.userSearch.searchBox.set('test'); + await takeComponentScreenshot(assert); + await component.instructor.userSearch.results.items[0].click(); assert.strictEqual(component.instructor.selectedInstructor, 'Aardvark'); }); + test('can search for course', async function (assert) { + const school = this.server.create('school', { title: 'first' }); + this.server.createList('course', 3, { school }); + await render( + , + ); + assert.ok(component.course.isVisible); + await component.course.input('course'); + await takeComponentScreenshot(assert); + assert.deepEqual(component.course.results.length, 3); + assert.deepEqual(component.course.results[0].text, '2013 course 0'); + assert.deepEqual(component.course.results[1].text, '2013 course 1'); + assert.deepEqual(component.course.results[2].text, '2013 course 2'); + }); + test('cancel', async function (assert) { const school = this.server.create('school', { title: 'first' }); const user = this.server.create('user', { school }); @@ -497,6 +528,7 @@ module('Integration | Component | reports/new-subject', function (hooks) { await component.title.set(this.title.repeat(25)); await component.save(); assert.strictEqual(component.title.error, 'Title is too long (maximum is 240 characters)'); + await takeComponentScreenshot(assert); assert.verifySteps(['setTitle called']); }); From 3b5c8ac7d053630ed4f48699bff4ca6de98c9e01 Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Fri, 6 Mar 2026 13:49:54 -0800 Subject: [PATCH 3/6] Allow Negative Margin on Breadcrumbs Specifically allowing this as it's how the arrows fit back together and it's internal to the component. --- .../components/instructor-group/header-test.gjs | 7 ++++++- .../app/styles/ilios-common/components/breadcrumbs.scss | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/frontend/tests/integration/components/instructor-group/header-test.gjs b/packages/frontend/tests/integration/components/instructor-group/header-test.gjs index 01d8997d09..3178028361 100644 --- a/packages/frontend/tests/integration/components/instructor-group/header-test.gjs +++ b/packages/frontend/tests/integration/components/instructor-group/header-test.gjs @@ -1,5 +1,5 @@ import { module, test } from 'qunit'; -import { setupRenderingTest } from 'frontend/tests/helpers'; +import { setupRenderingTest, takeComponentScreenshot } from 'frontend/tests/helpers'; import { setupMirage } from 'frontend/tests/test-support/mirage'; import { render } from '@ember/test-helpers'; import a11yAudit from 'ember-a11y-testing/test-support/audit'; @@ -32,6 +32,7 @@ module('Integration | Component | instructor-group/header', function (hooks) {
, ); + await takeComponentScreenshot(assert); assert.strictEqual(component.title.text, 'lorem ipsum'); assert.ok(component.title.isEditable); @@ -54,6 +55,7 @@ module('Integration | Component | instructor-group/header', function (hooks) { , ); + await takeComponentScreenshot(assert); assert.strictEqual(component.title.text, 'lorem ipsum'); assert.notOk(component.title.isEditable); assert.ok(component.members, 'Members: 3'); @@ -96,6 +98,7 @@ module('Integration | Component | instructor-group/header', function (hooks) { assert.strictEqual(component.title.value, 'lorem ipsum'); await component.title.set('a'.repeat(61)); await component.title.save(); + await takeComponentScreenshot(assert); assert.strictEqual(component.title.error, 'Title is too long (maximum is 60 characters)'); }); @@ -115,6 +118,7 @@ module('Integration | Component | instructor-group/header', function (hooks) { assert.strictEqual(component.title.value, 'lorem ipsum'); await component.title.set('AB'); await component.title.save(); + await takeComponentScreenshot(assert); assert.strictEqual(component.title.error, 'Title is too short (minimum is 3 characters)'); }); @@ -134,6 +138,7 @@ module('Integration | Component | instructor-group/header', function (hooks) { assert.strictEqual(component.title.value, 'lorem ipsum'); await component.title.set(''); await component.title.save(); + await takeComponentScreenshot(assert); assert.strictEqual(component.title.error, 'Title is too short (minimum is 3 characters)'); }); diff --git a/packages/ilios-common/app/styles/ilios-common/components/breadcrumbs.scss b/packages/ilios-common/app/styles/ilios-common/components/breadcrumbs.scss index ea06b291a2..fcb03e7092 100644 --- a/packages/ilios-common/app/styles/ilios-common/components/breadcrumbs.scss +++ b/packages/ilios-common/app/styles/ilios-common/components/breadcrumbs.scss @@ -16,7 +16,7 @@ } } - /* stylelint-disable property-disallowed-list */ + /* stylelint-disable property-disallowed-list, declaration-property-value-disallowed-list */ span { background-color: var(--white); border: $breadcrumb-border; From ba87e02f5fc897b2c964fcba550294cc23a0ac1a Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Fri, 6 Mar 2026 13:54:21 -0800 Subject: [PATCH 4/6] Remove Negative Margin from Legend This subtle alignment tweak doesn't hold up everywhere. Removing it wholesale, we can put it back later if needed. --- .../app/styles/ilios-common/mixins/ilios-fieldset.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ilios-common/app/styles/ilios-common/mixins/ilios-fieldset.scss b/packages/ilios-common/app/styles/ilios-common/mixins/ilios-fieldset.scss index 18e49ece86..77f4cf87f4 100644 --- a/packages/ilios-common/app/styles/ilios-common/mixins/ilios-fieldset.scss +++ b/packages/ilios-common/app/styles/ilios-common/mixins/ilios-fieldset.scss @@ -7,7 +7,7 @@ legend { @include ilios-heading.ilios-heading; font-weight: 600; - margin: 0 -0.5rem; + margin: 0; padding: 0 0.5rem; } } From 2582131177f6c5bd3d8c1e6c8e4b5fb66a2cdb76 Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Fri, 6 Mar 2026 13:56:37 -0800 Subject: [PATCH 5/6] Allow Visually Hidden Negative Margin This won't intrude on other components as it's positioned absolutely, plus this incantation is kind of a standard and works as is I'm not included to mess with it. --- .../app/styles/ilios-common/shared/visually-hidden.scss | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/ilios-common/app/styles/ilios-common/shared/visually-hidden.scss b/packages/ilios-common/app/styles/ilios-common/shared/visually-hidden.scss index c9025d88ba..4abff005a1 100644 --- a/packages/ilios-common/app/styles/ilios-common/shared/visually-hidden.scss +++ b/packages/ilios-common/app/styles/ilios-common/shared/visually-hidden.scss @@ -1,3 +1,4 @@ +/* stylelint-disable declaration-property-value-disallowed-list */ .visually-hidden { border: 0; clip-path: rect(0 0 0 0); From 705460dc296686c8955220eeb85c3e8d23ef4924 Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Fri, 6 Mar 2026 15:22:37 -0800 Subject: [PATCH 6/6] Find Component Screenshot Elements Better Sometimes (like when we use a {{let}} at the top of a template) the first element we encounter isn't the container we want to screenshot. In that case we can actually keep looking and probably find a good one to use. --- packages/frontend/tests/helpers/take-screenshot.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/packages/frontend/tests/helpers/take-screenshot.js b/packages/frontend/tests/helpers/take-screenshot.js index e806bdbac3..b1de7210cb 100644 --- a/packages/frontend/tests/helpers/take-screenshot.js +++ b/packages/frontend/tests/helpers/take-screenshot.js @@ -20,7 +20,16 @@ export const takeComponentScreenshot = async (assert, description = '') => { } const filename = getUniqueName(assert, description); const testing = document.getElementById('ember-testing'); - const element = testing.firstChild; + let element; + let i = 0; + do { + element = testing.children[i]; + i++; + } while (element && !(element instanceof Element)); + if (!(element instanceof Element)) { + console.error('Unable to find a valid Element to screenshot'); + return; + } return snap(element, filename, { backgroundColor: 'hsl(0, 0%, 98%)', });