refactor: rewrite breadcrumbs to use Lit and add tests #32
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,3 +14,5 @@ dist/ | |
|
|
||
| # os | ||
| .DS_Store | ||
|
|
||
| .nyc_output | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,20 +1,27 @@ | ||
| import { LitElement, html } from 'lit'; | ||
|
|
||
| const separator = html`<span class="select-none" aria-hidden="true">/</span>`; | ||
| const interleave = (arr) => | ||
| [].concat(...arr.map((el) => [el, separator])).slice(0, -1); | ||
|
|
||
| class FabricBreadcrumbs extends LitElement { | ||
| connectedCallback() { | ||
| super.connectedCallback(); | ||
| // Grab existing children at the point that the component is added to the page | ||
| // Interleave "/" separator with breadcrumbs | ||
| this._children = interleave(Array.from(this.children)); | ||
| } | ||
|
There was a problem hiding this comment. intercept the child elements when the component is first added to the page and interleave them with slash character separators. Store the result as the internal property _children for use in the render method. There was a problem hiding this comment. This interleaving is reactive in Vue (and I think React), but not here. Are we OK with that? Should that be disclosed? There was a problem hiding this comment. Unsure. It is perhaps indicative of one of the differences between react/vue and custom elements. I toyed around with using dedicated child elements instead of this which is maybe one solution. There was a problem hiding this comment. I'm fine with us making a call that people won't treat the children as reactive and fixing it later. I think this is a lot easier to get into trouble with in Vue/React than it is manually monkeying with DOM elements. But I defer to your judgement here! There was a problem hiding this comment. When you say not reactive do you mean in the context of using this custom element in React or Vue, or as in if the user updates the props, it wouldn't update the DOM element? @pearofducks There was a problem hiding this comment. I mean this implementation in the custom element isn't watching children - so if the user adds/removes breadcrumbs it won't re-interleave them. |
||
|
|
||
| render() { | ||
| return html` | ||
| <style> | ||
| @import "https://assets.finn.no/pkg/@fabric-ds/css/v1/fabric.min.css"; | ||
| </style> | ||
| <nav aria-label="Her er du" class="flex space-x-8"> | ||
| <h2 class="sr-only">Her er du</h2> | ||
| ${this._children} | ||
| </nav> | ||
| `; | ||
|
There was a problem hiding this comment. The render method ignores the original children and instead renders out the interleaved ones stored in this._children |
||
| } | ||
| } | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| /* eslint-disable no-undef */ | ||
| import tap, { test, beforeEach, teardown } from 'tap'; | ||
| import { chromium } from 'playwright'; | ||
|
|
||
| tap.before(async () => { | ||
| const browser = await chromium.launch({ headless: true }); | ||
| tap.context.browser = browser; | ||
| }); | ||
|
|
||
| beforeEach(async (t) => { | ||
| const { browser } = t.context; | ||
| const context = await browser.newContext(); | ||
| // context.setDefaultTimeout(2000); | ||
| t.context.page = await context.newPage(); | ||
| }); | ||
|
|
||
| teardown(async () => { | ||
| const { browser } = tap.context; | ||
| browser.close(); | ||
| }); | ||
|
|
||
| test('Breadcrumb component renders on the page', async (t) => { | ||
| // GIVEN: A component with 1 breadcrumb | ||
| const component = ` | ||
| <f-breadcrumbs> | ||
| <a href="#/url/1">Eiendom</a> | ||
| </f-breadcrumbs> | ||
| `; | ||
|
|
||
| // WHEN: the component is added to the page | ||
| const { page } = t.context; | ||
| await page.setContent(component); | ||
| await page.addScriptTag({ path: './dist/index.js', type: 'module' }); | ||
|
|
||
| // THEN: the component is visible in the DOM | ||
| t.equal(await page.innerText('nav a'), 'Eiendom', 'An Eiendom a tag should be added to the page'); | ||
| }); | ||
|
|
||
| test('Breadcrumb component interleaves / characters between breadcrumb items', async (t) => { | ||
| // GIVEN: A component with 2 breadcrumbs | ||
| const component = ` | ||
| <f-breadcrumbs> | ||
| <a href="#/url/1">Eiendom</a> | ||
| <a href="#/url/2">Torget</a> | ||
| </f-breadcrumbs> | ||
| `; | ||
|
|
||
| // WHEN: the component is added to the page AND spans are selected | ||
| const { page } = t.context; | ||
| await page.setContent(component); | ||
| await page.addScriptTag({ path: './dist/index.js', type: 'module' }); | ||
|
|
||
| // THEN: a single divider should have been interleaved with the breadcrumbs | ||
| t.equal(await page.innerText('f-breadcrumbs span'), '/', 'Divider slashes should be added'); | ||
| }); | ||
|
|
||
| test('Breadcrumb component with anchor child elements', async (t) => { | ||
| // GIVEN: A component with 3 breadcrumbs | ||
| const component = ` | ||
| <f-breadcrumbs> | ||
| <a href="#/url/1">Eiendom</a> | ||
| <a href="#/url/2">Torget</a> | ||
| <a href="#/url/3">Oslo</a> | ||
| </f-breadcrumbs> | ||
| `; | ||
|
|
||
| // WHEN: the component is added to the page AND a elements are selected | ||
| const { page } = t.context; | ||
| await page.setContent(component); | ||
| await page.addScriptTag({ path: './dist/index.js', type: 'module' }); | ||
|
|
||
| // THEN: there should be three breadcrumbs in the DOM | ||
| t.equal(await page.locator('f-breadcrumbs a').count(), 3, '3 a tags should be present'); | ||
| t.equal(await page.locator('f-breadcrumbs span').count(), 2, '2 span tags should be present'); | ||
| t.match( | ||
| await page.innerText(':nth-match(f-breadcrumbs a, 1)'), | ||
| 'Eiendom', | ||
| 'The first segment should be Eiendom', | ||
| ); | ||
| t.match( | ||
| await page.innerText(':nth-match(f-breadcrumbs a, 2)'), | ||
| 'Torget', | ||
| 'The second segment should be Torget', | ||
| ); | ||
| t.match( | ||
| await page.innerText(':nth-match(f-breadcrumbs a, 3)'), | ||
| 'Oslo', | ||
| 'The third segment should be Oslo', | ||
| ); | ||
| }); | ||
|
|
||
| test('Breadcrumb component with last element as a span', async (t) => { | ||
| // GIVEN: A component with 3 breadcrumbs | ||
| const component = ` | ||
| <f-breadcrumbs> | ||
| <a href="#/url/1">Eiendom</a> | ||
| <a href="#/url/2">Torget</a> | ||
| <span aria-current="page">Oslo</span> | ||
| </f-breadcrumbs> | ||
| `; | ||
|
|
||
| // WHEN: the component is added to the page AND a elements are selected | ||
| const { page } = t.context; | ||
| await page.setContent(component); | ||
| await page.addScriptTag({ path: './dist/index.js', type: 'module' }); | ||
|
|
||
| // THEN: there should be three breadcrumbs in the DOM | ||
| t.equal(await page.locator('f-breadcrumbs a').count(), 2, '2 child a tags should be present'); | ||
| t.equal( | ||
| await page.locator('f-breadcrumbs span').count(), | ||
| 3, | ||
| '3 child span tags should be present', | ||
| ); | ||
| }); | ||
|
There was a problem hiding this comment. Tests look how you would expect but are run (by web test runner) in a browser context so that |
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats the purpose of the
[].concathere?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map creates pairs of elements and separators
[el, separator] [el, separator] [el, separator], concat joins these array pairs together so you end up with 1 array with[el, separator, el, separator, el, separator]. Then the slice trims off the last separator so you end up with[el, separator, el, separator, el]There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't have to support Edge 16-18 or iOS11 we could instead
array.flatMap(el => [el, separator]).slice(1)