feat: introduce more customizable skip-nav #208
Conversation
…can be more customizable
|
|
||
| class NavigationSkipMain extends FocusMixin(LocalizeNavigationElement(LitElement)) { | ||
|
|
||
| static get focusElementSelector() { |
There was a problem hiding this comment.
While I was at it, I made both of these FocuxMixin-enabled, so calling focus() on the component will delegate it to the underlying <a>.
| return html`<d2l-navigation-skip text="${this.localize('skipNav')}" @click="${this._handleSkipNav}" class="vdiff-target"></d2l-navigation-skip>`; | ||
| } | ||
|
|
||
| _handleSkipNav() { |
There was a problem hiding this comment.
This is just the existing logic moved here.
|
|
||
| static get styles() { | ||
| return css` | ||
| a { | ||
| left: -10000px; | ||
| inset-inline-start: -10000px; |
There was a problem hiding this comment.
Removes the need for RtlMixin.
| render() { | ||
| return html`<a tabindex="0" @click="${this._handleSkipNav}" @keydown="${this._handleKeyDown}">${this.localize('skipNav')}</a>`; | ||
| return html`<a tabindex="0" @keydown="${this._handleKeyDown}" class="vdiff-target">${this.text}</a>`; |
There was a problem hiding this comment.
Now the mouse click event just bubbles up as if it originated from this component and pressing ENTER also fires that event.
|
|
||
| }); | ||
|
|
||
| describe('skip logic', () => { |
There was a problem hiding this comment.
There wasn't anything testing this logic before, so I added it.
|
|
||
| describe('d2l-navigation-skip-main', () => { | ||
|
|
||
| ['en', 'ar'].forEach(lang => { |
There was a problem hiding this comment.
Just testing Arabic here to exercise the RTL logic.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Do you think it's worth adding tests that check the focus does go where we expect? It seems like the unit tests cover these pretty well, maybe just one of those cases where either unit or vdiff would work
There was a problem hiding this comment.
d2l-navigation-skip doesn't have any focus logic, it just fires the event. But yeah, d2l-navigation-skip-main's unit tests cover all its focus logic, and I did originally have that in the vdiff but since it was testable from a unit test I opted to do it there to speed things up.
| } | ||
|
|
||
| render() { | ||
| return html`<d2l-navigation-skip text="${this.localize('skipNav')}" @click="${this._handleSkipNav}" class="vdiff-target"></d2l-navigation-skip>`; |
There was a problem hiding this comment.
Love seeing the vdiff-target stuff just work!
There was a problem hiding this comment.
I know! I had to refer to the docs to make sure I did it right, which also just worked. 🎉
|
🎉 This PR is included in version 5.16.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
A case recently came up where a more customizable skip-nav link was required. Both the text needed to be customized as well as the focus logic when activated.
To solve this, I've renamed the existing
element" functionality. That leavesd2l-navigation-skipcomponent tod2l-navigation-skip-mainto reflect its "skip to main content" text and "focus on thed2l-navigation-skipavailable to become a more customizable version.d2l-navigation-skipwas only ever used internally to this repo, so no migration is required.