Use es6 class syntax #376

mxmason · 2022-07-01T17:24:49Z

Summary

Refactors the source file to use ES6 class syntax
Updates the typedef file accordingly
Refactors the rollup config

KittyGiraudel

Really cool! ✨ I left a lot of comments but most of them are about bringing a bit of spacing around some code with empty lines. :)

KittyGiraudel · 2022-07-02T09:26:44Z

src/a11y-dialog.js

+const TAB_KEY = 'Tab'
+const ESCAPE_KEY = 'Escape'


Also wondering whether we should just call these TAB and ESCAPE. This can be done separately though.

KittyGiraudel · 2022-07-02T09:27:13Z

src/a11y-dialog.js

+ * @typedef { import ('./a11y-dialog').A11yDialog } A11yDialogType
+ * @typedef { import('./a11y-dialog').EventType } EventType
+ * @typedef { import('./a11y-dialog').EventHandler } EventHandler
+ * @typedef { import('./a11y-dialog').ListenersRecord } ListenersRecord


So what’s any of this? 😹

Force of habit? 🤣 This is JSDoc syntax that imports the types defined in the d.ts file, so they can be referenced with the @param keyword. It’s not necessary since we know TypeScript is coming soon, though

src/a11y-dialog.js

KittyGiraudel · 2022-07-02T09:28:43Z

src/a11y-dialog.js


-  return this
-}
+    this.previouslyFocused = null


I kind of liked to have a leading underscore for properties and methods that are not part of the public API. What do you think?

I like it when we have no way in a language to indicate privacy to the dev, but we can use the keyword “private” in TypeScript, so I think I would prefer not to have leading underscores since we’re about to migrate.

We’ll need to clarify the name change in the changelog though, since some people might be relying on these “hidden” properties.

src/a11y-dialog.js

KittyGiraudel · 2022-07-02T09:34:43Z

src/a11y-dialog.js

-  // stays trapped within the dialog element
-  if (this.shown && event.key === TAB_KEY) {
-    trapTabKey(this.$el, event)
+  /**


Suggested change

/**

/**

KittyGiraudel · 2022-07-02T09:35:11Z

src/a11y-dialog.js

+    // This is an escape hatch in case there are nested dialogs, so the keypresses
+    // are only reacted to for the most recent one
+    if (!this.$el.contains(document.activeElement)) return
+    // If the dialog is shown and the ESC key is pressed, prevent any further


Suggested change

// If the dialog is shown and the ESC key is pressed, prevent any further

// If the dialog is shown and the ESC key is pressed, prevent any further

KittyGiraudel · 2022-07-02T09:35:41Z

src/a11y-dialog.js

+      event.preventDefault()
+      this.hide(event)
+    }
+    // If the dialog is shown and the TAB key is pressed, make sure the focus


Suggested change

// If the dialog is shown and the TAB key is pressed, make sure the focus

// If the dialog is shown and the TAB key is pressed, make sure the focus

KittyGiraudel · 2022-07-02T09:38:27Z

rollup.config.js

-const minify = {
-  plugins: [terser()],
-  banner: () => `/*! a11y-dialog ${pkg.version} — © Kitty Giraudel */`,
+const babelCfg = {


I need to spend a bit of time digesting this config. It’s been a while I haven’t played with Babel. :)

KittyGiraudel · 2022-07-02T09:38:37Z

rollup.config.js

-const minify = {
-  plugins: [terser()],
-  banner: () => `/*! a11y-dialog ${pkg.version} — © Kitty Giraudel */`,
+const babelCfg = {


I need to spend a bit of time digesting this config. It’s been a while I haven’t played with Babel. :)

tomasvn · 2022-07-03T21:32:16Z

rollup.config.js

+    noClassCalls: true,
+    noDocumentAll: true,
+  },
+  presets: [['@babel/preset-env', { targets: 'ie 11' }]],


Is it really needed to transpile to IE11? As IE11 is out of support, when opening IE11 it suggets to download edge. @KittyGiraudel @mxmason

Hi, @tomasvn! IE 11 has reached its end-of-life, but it might also be the case that certain enterprises have an extended support agreement, and will still be using IE 11. I see two paths forward:

Drop support for IE 11 in version 8 of a11y-dialog; direct IE users to version 7

Continue support for IE 11 in version 8 of a11y-dialog, keeping the PR as-is

Ultimately, I think the right path forward should be @KittyGiraudel's decision.

I think it would be interesting to know the size difference with or without IE11 support in the ES5. If the difference is marginal, we should keep it because why not. If it’s significant, maybe we could drop it and refer people to v7.

Downleveling es6 to es5 makes the non-esm build about 0.2kb bigger than it used to be, and 0.3kb bigger than the ESM build that this branch produces.

ESM: 1.2kb (currently 1.3kb in prod)
UMD: 1.5kb (currently 1.3kb in prod)

I'm comfortable with shipping the additional kb to maintain support if you are. It's no real cost to our developer experience, as far as I can see it.

Yep, let’s keep IE11 support. That’s marginal if anything.

KittyGiraudel · 2022-07-07T07:20:55Z

@mxmason How do you feel? Shall we merge it? :)

mxmason · 2022-07-07T08:01:54Z

@KittyGiraudel I'm good to merge this in if you are. I think I can move on to a TypeScript migration next, and then perhaps some small implementation changes, like the delegation you mentioned.

KittyGiraudel · 2022-07-07T08:02:57Z

Yaaaay! ✨

mxmason added 5 commits July 1, 2022 09:27

feat: use es6 class syntax

d6b8acd

chore: update type file

7d60843

chore: add build dependencies

55f269c

chore: update rollup config

c6f7e51

chore: recreate lockfile

7a45eaf

mxmason force-pushed the feat/es6-classes branch from 20b28d6 to 7a45eaf Compare July 1, 2022 17:29

Merge remote-tracking branch 'upstream/main' into feat/es6-classes

d63276d

mxmason marked this pull request as ready for review July 1, 2022 17:35

KittyGiraudel reviewed Jul 2, 2022

View reviewed changes

mxmason added 5 commits July 2, 2022 09:38

chore: flatten type file

a86c453

chore: clean up spacing

e632b29

chore: more cleanup

481464a

chore: remove unwanted call to create

9099bb0

chore: fix event type

61a24bf

tomasvn reviewed Jul 3, 2022

View reviewed changes

KittyGiraudel mentioned this pull request Jul 5, 2022

Refactor with ES6 class syntax #373

Closed

mxmason added 3 commits July 6, 2022 18:51

Merge remote-tracking branch 'upstream/main' into feat/es6-classes

f2d0641

chore: fix arg types

aa493b0

chore: use optional chaining

7b6d6d6

KittyGiraudel merged commit 30dd29a into KittyGiraudel:main Jul 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use es6 class syntax #376

Use es6 class syntax #376

mxmason commented Jul 1, 2022 •

edited

Loading

KittyGiraudel left a comment

KittyGiraudel Jul 2, 2022

KittyGiraudel Jul 2, 2022

mxmason Jul 2, 2022

KittyGiraudel Jul 2, 2022

mxmason Jul 2, 2022

KittyGiraudel Jul 3, 2022

KittyGiraudel Jul 2, 2022

KittyGiraudel Jul 2, 2022

KittyGiraudel Jul 2, 2022

KittyGiraudel Jul 2, 2022

KittyGiraudel Jul 2, 2022

tomasvn Jul 3, 2022

mxmason Jul 6, 2022

KittyGiraudel Jul 6, 2022

mxmason Jul 7, 2022

KittyGiraudel Jul 7, 2022

KittyGiraudel commented Jul 7, 2022

mxmason commented Jul 7, 2022

KittyGiraudel commented Jul 7, 2022

	// If the dialog is shown and the ESC key is pressed, prevent any further

	// If the dialog is shown and the ESC key is pressed, prevent any further

	// If the dialog is shown and the TAB key is pressed, make sure the focus

	// If the dialog is shown and the TAB key is pressed, make sure the focus

Use es6 class syntax #376

Use es6 class syntax #376

Conversation

mxmason commented Jul 1, 2022 • edited Loading

Summary

KittyGiraudel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KittyGiraudel commented Jul 7, 2022

mxmason commented Jul 7, 2022

KittyGiraudel commented Jul 7, 2022

mxmason commented Jul 1, 2022 •

edited

Loading