test: components/breadcrumb-item #541
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/shoelace/shoelace/3Sq5p5NGtk1JEYXze7iunm1XhMXv |
| target="${this.target}" | ||
| rel=${ifDefined(this.target ? 'noreferrer noopener' : undefined)} | ||
| rel=${ifDefined(this.target ? rel : undefined)} |
There was a problem hiding this comment.
The "target" could potentially be wrapped in an ifDefined but not sure if that's over engineering. Currently it results in a target="" when not supplied, but that still passes AXE apparently.
| handleSlotChange() { | ||
| this.hasPrefix = hasSlot(this, 'prefix'); | ||
| this.hasSuffix = hasSlot(this, 'suffix'); | ||
| } | ||
|
|
||
| render() { | ||
| const isLink = this.href ? true : false; | ||
| const rel = this.rel ? this.rel : 'noreferrer noopener'; |
There was a problem hiding this comment.
I'm curious if there's a reason you're setting the default here instead of on line 40. I typically set the default in the prop's definition so the CEM analyzer picks it up and it gets shown in the docs.
There was a problem hiding this comment.
Because we wouldn't want it to be more succinct would we 🙄 /sarcasm 🤦🏼
|
Thanks so much for this! Everything looks good here, just one thing about |
|
Funnily enough having the test in place made it easy to make the change you picked up! 😅 Thanks for that, slight oversight on my end. |
I made a change here that users should be able to control the "rel" attribute when it's supplied.
Test suite itself, non-eventful.