Skip to content

INSTUI-4672 Add the (almost complete) regression test suite, fix small a11y issues, SSR errors #2105

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
matyasf merged 1 commit into from
Aug 29, 2025
Merged

INSTUI-4672 Add the (almost complete) regression test suite, fix small a11y issues, SSR errors #2105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
"@types/react-dom": "^18.3.0",
"@vitejs/plugin-react": "^4.5.1",
"@vitest/eslint-plugin": "^1.2.1",
"@types/node": "^22",
Copy link
Collaborator Author

@matyasf matyasf Aug 18, 2025

Choose a reason for hiding this comment

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

This must be added otherwise it will just pick up a @types/node version from a dependency

"chai": "^4.4.1",
"chalk": "^4.1.2",
"commitizen": "^4.3.1",
Expand Down Expand Up @@ -101,7 +102,7 @@
"@types/jest": "needed because https://github.com/testing-library/jest-dom/issues/544 recheck if fixed"
},
"engines": {
"node": ">=18",
"node": ">=22",
Copy link
Collaborator Author

@matyasf matyasf Aug 18, 2025

Choose a reason for hiding this comment

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

I've updated this to be in sync with what we use in Github actions

"yarn": "YARN NO LONGER USED - use npm instead."
},
"config": {
Expand Down
18 changes: 10 additions & 8 deletions packages/ui-calendar/src/Calendar/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@ import { View } from '@instructure/ui-view'
import {
safeCloneElement,
callRenderProp,
omitProps
omitProps,
withDeterministicId
} from '@instructure/ui-react-utils'
import { createChainedFunction } from '@instructure/ui-utils'
import { logError as error } from '@instructure/console'
import { uid } from '@instructure/uid'
import { AccessibleContent } from '@instructure/ui-a11y-content'

import { testable } from '@instructure/ui-testable'
Expand Down Expand Up @@ -64,6 +64,7 @@ import { SimpleSelect } from '@instructure/ui-simple-select'
category: components
---
**/
@withDeterministicId()
@withStyle(generateStyle, generateComponentTheme)
@testable()
class Calendar extends Component<CalendarProps, CalendarState> {
Expand All @@ -82,14 +83,15 @@ class Calendar extends Component<CalendarProps, CalendarState> {
}

ref: Element | null = null
private _weekdayHeaderIds = (
this.props.renderWeekdayLabels || this.defaultWeekdays
).reduce((ids: Record<number, string>, _label, i) => {
return { ...ids, [i]: uid(`weekday-header-${i}`) }
}, {})
private _weekdayHeaderIds

constructor(props: CalendarProps) {
super(props)

this._weekdayHeaderIds = (
this.props.renderWeekdayLabels || this.defaultWeekdays
).reduce((ids: Record<number, string>, _label, i) => {
return { ...ids, [i]: this.props.deterministicId!('weekday-header') }
}, {})
Comment on lines +90 to +94
Copy link
Collaborator Author

@matyasf matyasf Aug 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

SSR bugfix due to ID not being deterministic

this.state = this.calculateState(
this.locale(),
this.timezone(),
Expand Down
4 changes: 3 additions & 1 deletion packages/ui-calendar/src/Calendar/props.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import { ReactElement } from 'react'
import type { CalendarDayProps } from './Day/props'
import { Renderable } from '@instructure/shared-types'
import type { Moment } from '@instructure/ui-i18n'
import type { WithDeterministicIdProps } from '@instructure/ui-react-utils'

type CalendarOwnProps = {
/**
Expand Down Expand Up @@ -178,7 +179,8 @@ type AllowedPropKeys = Readonly<Array<PropKeys>>

type CalendarProps = CalendarOwnProps &
WithStyleProps<CalendarTheme, CalendarStyle> &
OtherHTMLAttributes<CalendarOwnProps>
OtherHTMLAttributes<CalendarOwnProps> &
WithDeterministicIdProps

type CalendarStyle = ComponentStyle<
'navigation' | 'navigationWithButtons' | 'weekdayHeader' | 'yearPicker'
Expand Down
13 changes: 9 additions & 4 deletions packages/ui-codemods/lib/__node_tests__/runTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,10 @@ export function runTest(codemod: Transform) {
entries.forEach((entry) => {
if (entry.isFile() && entry.name.includes('input')) {
const isWarningTest = entry.name.includes('.warning.input')
const inputPath = path.join(entry.path, entry.name)
const inputPath = path.join(entry.parentPath, entry.name)
Copy link
Collaborator Author

@matyasf matyasf Aug 18, 2025

Choose a reason for hiding this comment

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

entry.path is removed in Node 24, this is the recommended way since Node 20

const input = fs.readFileSync(inputPath, 'utf8')
const expectedName = entry.name.replace('input', 'output')
const expectedPath = path.join(entry.path, expectedName)
const expectedPath = path.join(entry.parentPath, expectedName)
const expected = fs.readFileSync(expectedPath, 'utf8')

// eslint-disable-next-line no-console
Expand All @@ -61,12 +61,17 @@ export function runTest(codemod: Transform) {
)

if (isWarningTest) {
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => { })
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {})

try {
const j = jscodeshift.withParser('tsx')
const fileInfo = { path: inputPath, source: input }
const api = { jscodeshift: j, j: j, stats: () => { }, report: () => { } }
const api = {
jscodeshift: j,
j: j,
stats: () => {},
report: () => {}
}
const options = {}

// Run codemod withouth comparison
Expand Down
2 changes: 1 addition & 1 deletion packages/ui-drilldown/src/Drilldown/DrilldownOption/__tests__/DrilldownOption.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ describe('<Drilldown.Option />', () => {
)
const option = screen.getByLabelText('Option')

expect(option).toHaveAttribute('role', 'button')
expect(option).toHaveAttribute('role', 'menuitem')
expect(option).toHaveAttribute('aria-haspopup', 'true')
})
})
Expand Down
2 changes: 1 addition & 1 deletion packages/ui-drilldown/src/Drilldown/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1126,7 +1126,7 @@ class Drilldown extends Component<DrilldownProps, DrilldownState> {
if (subPageId) {
optionProps.renderAfterLabel = <IconArrowOpenEndSolid />
optionProps['aria-haspopup'] = true
optionProps.role = customRole || 'button'
optionProps.role = customRole || 'menuitem'
Copy link
Collaborator Author

@matyasf matyasf Aug 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Drilldown has role=menu, which should not have role=button children. We had a similar issue in the past: #1831 Also see the W3 spec: https://w3c.github.io/aria/#menu . Also this triggers a critical axe check failure (on our docs page too)

warn(
!renderAfterLabel,
`The prop "renderAfterLabel" is reserved on item with id: "${id}". When it has "subPageId" provided, a navigation arrow will render after the label.`
Expand Down
6 changes: 3 additions & 3 deletions packages/ui-heading/src/Heading/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@ Pre-configured and with unique styles, the `ai-headings` are used for standardiz
type: example
---
<div style={{display: 'flex', flexDirection: 'column', gap: '24px'}}>
<Heading aiVariant="stacked">Nutrition Facts</Heading>
<Heading aiVariant="horizontal">Nutrition Facts</Heading>
<Heading aiVariant="iconOnly">Nutrition Facts</Heading>
<Heading aiVariant="stacked" level="h2">Nutrition Facts</Heading>
<Heading aiVariant="horizontal" level="h3">Nutrition Facts</Heading>
<Heading aiVariant="iconOnly" level="h4">Nutrition Facts</Heading>
Comment on lines +52 to +54
Copy link
Collaborator Author

@matyasf matyasf Aug 18, 2025

Choose a reason for hiding this comment

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

variant should be specified according to our own docs

Copy link
Contributor

@joyenjoyer joyenjoyer Aug 28, 2025

Choose a reason for hiding this comment

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

You mean level instead of variant I guess.

</div>
```

Expand Down
5 changes: 2 additions & 3 deletions packages/ui-options/src/Options/Item/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,11 @@ const generateStyle = (
background: componentTheme.selectedBackground,
color: componentTheme.highlightedLabelColor
},
disabled: { cursor: 'not-allowed', opacity: 0.5 },
disabled: { cursor: 'not-allowed', opacity: 0.68 },
'highlighted-disabled': {
background: componentTheme.highlightedBackground,
color: componentTheme.highlightedLabelColor,
cursor: 'not-allowed',
opacity: 0.5
cursor: 'not-allowed'
Comment on lines +66 to +70
Copy link
Collaborator Author

@matyasf matyasf Aug 18, 2025

Choose a reason for hiding this comment

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

These needed the opacity to be increased, otherwise they would fail color contract a11y tests. AFAIK we are not using this directly anywhere, so it should be OK

},
'selected-highlighted': {
background: componentTheme.selectedHighlightedBackground,
Expand Down
10 changes: 8 additions & 2 deletions packages/ui-select/src/Select/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -780,8 +780,14 @@ class Select extends Component<SelectProps> {
onBlur: utils.createChainedFunction(onBlur, onRequestHideOptions),
...overrideProps
}

return <TextInput {...triggerProps} {...getInputProps(inputProps)} />
// suppressHydrationWarning is needed because `role` depends on the browser type
return (
<TextInput
{...triggerProps}
{...getInputProps(inputProps)}
suppressHydrationWarning
/>
)
}

render() {
Expand Down
4 changes: 2 additions & 2 deletions ...avBar/TopNavBarLayout/SmallViewportLayout/__tests__/TopNavBarSmallViewportLayout.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,7 +425,7 @@ describe('<TopNavBarSmallViewportLayout />', () => {
const dropdownMenu = await screen.findByRole('menu')
expect(dropdownMenu).toBeInTheDocument()

const option = await within(dropdownMenu).findByRole('button')
const option = await within(dropdownMenu).findByRole('menuitem')
expect(option).toHaveAttribute('id', `TestItem1`)

fireEvent.click(option)
Expand Down Expand Up @@ -467,7 +467,7 @@ describe('<TopNavBarSmallViewportLayout />', () => {
const dropdownMenu = await screen.findByRole('menu')
expect(dropdownMenu).toBeInTheDocument()

const option = await within(dropdownMenu).findByRole('button')
const option = await within(dropdownMenu).findByRole('menuitem')
expect(option).toHaveAttribute('id', `TestItem1`)

fireEvent.click(option)
Expand Down
2 changes: 1 addition & 1 deletion regression-test/cypress.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import { installPlugin } from '@chromatic-com/cypress'

export default defineConfig({
env: {
delay: 100
delay: 100 // Chromatic snapshot delay time
},
e2e: {
setupNodeEvents(on, config) {
Expand Down
Loading
Loading