feat: remove ember-prism in favour of ember-shiki #112

IgnaceMaes · 2023-08-28T10:06:48Z

No description provided.

app/components/code-snippet.css

ijlee2

The failing test can be fixed by ignoring the rule (a false positive imo, one that happens easily):

await a11yAudit({
  rules: {
    'scrollable-region-focusable': {
      enabled: false,
    },
  },
});

package.json

IgnaceMaes · 2023-08-28T11:22:34Z

[critical]: Buttons must have discernible text

Should be fixed in ember-shiki itself I presume. It would be good to set up an automated a11y test like here.

ijlee2 · 2023-08-28T12:15:54Z

@IgnaceMaes I wasn't able to run the app locally. Could you check what happened?

App is being served by FastBoot
ReferenceError: fetch is not defined
    at buildSandboxGlobals (/ember-octane-vs-classic-cheat-sheet/config/fastboot.js:7:16)
    at EmberApp.buildSandbox (/ember-octane-vs-classic-cheat-sheet/node_modules/fastboot/src/ember-app.js:120:19)
    at EmberApp.buildApp (/ember-octane-vs-classic-cheat-sheet/node_modules/fastboot/src/ember-app.js:152:24)
    at /ember-octane-vs-classic-cheat-sheet/node_modules/fastboot/src/ember-app.js:139:22
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async EmberApp.getNewApplicationInstance (/ember-octane-vs-classic-cheat-sheet/node_modules/fastboot/src/ember-app.js:209:17)
    at async EmberApp._visit (/ember-octane-vs-classic-cheat-sheet/node_modules/fastboot/src/ember-app.js:240:9)
    at async EmberApp.visit (/ember-octane-vs-classic-cheat-sheet/node_modules/fastboot/src/ember-app.js:325:7)
    at async FastBoot.visit (/ember-octane-vs-classic-cheat-sheet/node_modules/fastboot/src/index.js:86:18)
    at async /ember-octane-vs-classic-cheat-sheet/node_modules/fastboot-express-middleware/src/index.js:33:20

Is FastBoot somehow a dependency of ember-shiki? If the setup for local development needs to be updated somehow, I'd ask that you document the changes in the README or CONTRIBUTING.

IgnaceMaes · 2023-08-28T12:25:04Z

@IgnaceMaes I wasn't able to run the app locally. Could you check what happened?

App is being served by FastBoot
ReferenceError: fetch is not defined
    at buildSandboxGlobals (/ember-octane-vs-classic-cheat-sheet/config/fastboot.js:7:16)
    at EmberApp.buildSandbox (/ember-octane-vs-classic-cheat-sheet/node_modules/fastboot/src/ember-app.js:120:19)
    at EmberApp.buildApp (/ember-octane-vs-classic-cheat-sheet/node_modules/fastboot/src/ember-app.js:152:24)
    at /ember-octane-vs-classic-cheat-sheet/node_modules/fastboot/src/ember-app.js:139:22
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async EmberApp.getNewApplicationInstance (/ember-octane-vs-classic-cheat-sheet/node_modules/fastboot/src/ember-app.js:209:17)
    at async EmberApp._visit (/ember-octane-vs-classic-cheat-sheet/node_modules/fastboot/src/ember-app.js:240:9)
    at async EmberApp.visit (/ember-octane-vs-classic-cheat-sheet/node_modules/fastboot/src/ember-app.js:325:7)
    at async FastBoot.visit (/ember-octane-vs-classic-cheat-sheet/node_modules/fastboot/src/index.js:86:18)
    at async /ember-octane-vs-classic-cheat-sheet/node_modules/fastboot-express-middleware/src/index.js:33:20

Is FastBoot somehow a dependency of `ember-shiki`? If the setup for local development needs to be updated somehow, I'd ask that you document the changes in the `README` or `CONTRIBUTING`.

It's because the fastboot implementation uses fetch, but fetch was only introduced as a global in Node 18. (ref)

I've reverted to Node 16 and installed node-fetch as an alternative to not have to bump those requirements for now.

ijlee2

Sweet!

jenweber · 2023-08-28T15:46:54Z

Thank you! Here's a followup issue for tracking implementing the TS examples for the user experience: #113

jenweber

I like this step towards showing TS examples! I found one blocking issue. Can we try using a different color scheme? Previously, this app had 100% Lighthouse accessibility score, but there are 2 issues:

copy button lacks an accessible name. Do we have a way to fix that button name, maybe upstream?
the color contrast for this theme is too low, looks like specifically the light blue is a problem. Is there another theme we can use that will pass? Or maybe drop the syntax highlighting by file type in cases where the light blue shows up (like cli commands)

This brings the score down to 87%. Across our apps, we strive for meeting AA level accessibility criteria.

To re-run these tests, go to Chrome developer tools, choose "Lighthouse" and select the Accessibility option. Then click "Analyze page load." Sorry this isn't represented in CI tests! Let me know if you need any help.

IgnaceMaes · 2023-08-28T18:56:51Z

Thanks for the review @jenweber.

The button label has been added upstream in ember-shiki. Also set up a11y-testing to make sure it stays compliant 😄
Switched to dark-plus theme (the VS Code default) which meets AA accessibility. I'm personally less of a fan of it on the web, but hey if it meets the a11y requirements it does the job!

I haven't checked support of other themes so if anyone wants to try a different one, please go ahead!

jenweber

Awesome, thank you for the a11y fixes!

ijlee2 · 2023-09-01T14:15:06Z

@IgnaceMaes @jenweber The deployment of the app seems to have failed because fetch is missing. I'm not sure if switching to npm in #114 would help. Could you take a look at it some time?

https://github.com/ember-learn/ember-octane-vs-classic-cheat-sheet/actions/runs/6050042815/job/16418543130

jenweber · 2023-09-01T14:33:46Z

Oops, taking a look.

IgnaceMaes · 2023-09-01T14:49:12Z

Found the issue. It's because Prember only started loading the fastboot.js config since v2: https://github.com/ef4/prember/releases/tag/v2.0.0.

I'll open a PR to upgrade the dependency in a sec.

jenweber · 2023-09-01T14:51:49Z

Edit - as I was typing, @IgnaceMaes opened a PR! Thank you!

Just in case it's still useful somehow...

I'm a little surprised to see this error, since both ember-fetch and node-fetch are present.

Debugging:

npx ember build production does not surface this issue locally.
I can see in the CI deploy logs that we are using node 16
While running this app locally on node 16, I do not see any issues
I think updating npm is unlikely to help, but going to node 18 could

jenweber · 2023-09-01T15:06:29Z

After merging #115 the deploy succeeded. Thanks again @IgnaceMaes

ignace added 8 commits August 28, 2023 11:56

feat: use ember-shiki in favour of ember-prism

f646402

chore: clean up prism leftovers

24a49de

feat: add some spacing between code blocks

d49775d

feat: use github-dark theme

6837935

fix: use nodejs v18 for global fetch

38c966c

chore: make it a dev-dependency

a4dd5f0

fix: lint Headers as global

b773ab6

fix: add resolution for ember-modifier

c2a5f12

ijlee2 reviewed Aug 28, 2023

View reviewed changes

app/components/code-snippet.css Outdated Show resolved Hide resolved

ijlee2 reviewed Aug 28, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

ignace added 3 commits August 28, 2023 13:14

deps: update ember-container-query to v4

3daeab4

fix: disable scrollable-region-focusable a11y rule

f0f2ab7

fix: move codeblock margin to subsection component

748edcf

IgnaceMaes mentioned this pull request Aug 28, 2023

tests: set up a11y testing IgnaceMaes/ember-shiki#10

Closed

chore: Temporarily ignored the button-name rule

12c31df

chore: use node-fetch and revert to node v16

35ae4d3

IgnaceMaes changed the title ~~feat: use ember-shiki in favour of ember-prism~~ feat: remove ember-prism in favour of ember-shiki Aug 28, 2023

ijlee2 previously approved these changes Aug 28, 2023

View reviewed changes

jenweber mentioned this pull request Aug 28, 2023

Feature - provide TypeScript examples for code blocks #113

Open

jenweber requested changes Aug 28, 2023

View reviewed changes

fix: a11y button label

605ee6a

IgnaceMaes dismissed ijlee2’s stale review via 605ee6a August 28, 2023 18:47

chore: switch to dark-plus theme to meet a11y test

2698c2e

IgnaceMaes requested a review from jenweber August 28, 2023 18:58

ijlee2 approved these changes Aug 29, 2023

View reviewed changes

jenweber approved these changes Sep 1, 2023

View reviewed changes

jenweber merged commit 9f04032 into ember-learn:main Sep 1, 2023
3 checks passed

IgnaceMaes mentioned this pull request Sep 1, 2023

deps: upgrade prember to v2 to fix fastboot config loading #115

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: remove ember-prism in favour of ember-shiki #112

feat: remove ember-prism in favour of ember-shiki #112

IgnaceMaes commented Aug 28, 2023

ijlee2 left a comment

IgnaceMaes commented Aug 28, 2023

ijlee2 commented Aug 28, 2023

IgnaceMaes commented Aug 28, 2023

ijlee2 left a comment

jenweber commented Aug 28, 2023

jenweber left a comment •

edited

Loading

IgnaceMaes commented Aug 28, 2023

jenweber left a comment

ijlee2 commented Sep 1, 2023 •

edited

Loading

jenweber commented Sep 1, 2023

IgnaceMaes commented Sep 1, 2023

jenweber commented Sep 1, 2023

jenweber commented Sep 1, 2023

feat: remove ember-prism in favour of ember-shiki #112

feat: remove ember-prism in favour of ember-shiki #112

Conversation

IgnaceMaes commented Aug 28, 2023

ijlee2 left a comment

Choose a reason for hiding this comment

IgnaceMaes commented Aug 28, 2023

ijlee2 commented Aug 28, 2023

IgnaceMaes commented Aug 28, 2023

ijlee2 left a comment

Choose a reason for hiding this comment

jenweber commented Aug 28, 2023

jenweber left a comment • edited Loading

Choose a reason for hiding this comment

IgnaceMaes commented Aug 28, 2023

jenweber left a comment

Choose a reason for hiding this comment

ijlee2 commented Sep 1, 2023 • edited Loading

jenweber commented Sep 1, 2023

IgnaceMaes commented Sep 1, 2023

jenweber commented Sep 1, 2023

jenweber commented Sep 1, 2023

jenweber left a comment •

edited

Loading

ijlee2 commented Sep 1, 2023 •

edited

Loading