Clippy Link #454
Clippy Link #454
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #454 +/- ##
===========================================
- Coverage 100.00% 90.60% -9.40%
===========================================
Files 12 50 +38
Lines 694 1426 +732
Branches 100 309 +209
===========================================
+ Hits 694 1292 +598
- Misses 0 110 +110
- Partials 0 24 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
| }); | ||
|
|
||
| it('does not forward arbitrary anchor-specific attributes from the host', async () => { | ||
| document.body.innerHTML = `<${tag} download="file.pdf" hreflang="en" ping="https://example.com/ping" referrerpolicy="no-referrer" type="application/pdf"></${tag}>`; |
There was a problem hiding this comment.
Dit gedrag als default snap ik niet zo goed. Het lijkt mij juist logisch dat je de clippy-link als een soort drop-in wil kunnen gebruiken voor elke link, ook een download link of een link in een andere taal. Nu werkt dat niet totdat je in de documentatie hebt gevonden dat je een extra (clippy-link-specifieke) attribute moet meegeven. Wat los je op door dit standaard niet door te geven?
| ## Extra properties via `restProps` (property-only) | ||
|
|
||
| Sommige `<a>`-properties zijn beschikbaar via `restProps` (dit is **geen** attribute-API; alleen via JavaScript). Deze properties worden via property bindings doorgestuurd naar het onderliggende `<a>` element: |
There was a problem hiding this comment.
Hoe werkt dit dan? Ik zie het namelijk niet terug in de code.
Wat is het voordeel van ze in de restProps stoppen? Waarom zou je ze niet laten forwarden?
There was a problem hiding this comment.
oi oude implementatie en docs niet bijgewerkt! mijn fout
| .href=${t(`tokens.fieldLabels.basis.color.${colorKey}.docs`)} | ||
| .target=${"_blank"} |
There was a problem hiding this comment.
waarom kunnen dit geen standaard href en target zijn, waarom is die . nodig in deze context? Het lijken me beide gewoon simpele strings
|
|
||
| #handleNavClick(event: Event): void { | ||
| const link = (event.target as Element).closest('a[href^="#"]'); | ||
| const link = (event.target as Element).closest('clippy-link[href^="#"], a[href^="#"]'); |
There was a problem hiding this comment.
Hmm dit voelt als code smell, als we deze functionaliteit willen ondersteunen willen we (lijkt mij) niet alle mogelijke tags die we in de body gebruiken waar een href op kan zitten ondersteunen.
Als ik t goed begrijp vang je de click af, enkel om te scrollen naar het element. Zou dit dan niet genoeg zijn:
@media (prefers-reduced-motion: no-preference) {
html {
scroll-behavior: smooth;
}
}
https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/scroll-behavior
| <clippy-link inline-box class="wizard-styleguide__nav-item" href="#colors">${t('styleGuide.sections.colors.title')}</clippy-link> | ||
| <clippy-link inline-box class="wizard-styleguide__nav-item" href="#typography">${t('styleGuide.sections.typography.title')}</clippy-link> | ||
| <clippy-link inline-box class="wizard-styleguide__nav-item" href="#spacing">${t('styleGuide.sections.space.title')}</clippy-link> | ||
| <clippy-link inline-box class="wizard-styleguide__nav-item" href="#components">${t('styleGuide.sections.components.title')}</clippy-link> |
There was a problem hiding this comment.
Dit voelt mij ook een beetje aan als code smell. Nu het componenten zijn met specifieke styling zou ik verwachten dat ze (uiteindelijk ook) voldoen in deze context. Voor nu zou ik t wel zo laten, want ik verwacht dat misschien ook een eigen component wordt.
|
|
||
| <div> | ||
| <template-link-list | ||
| <clippy-link-list |
There was a problem hiding this comment.
Zit deze al in de repo?
There was a problem hiding this comment.
search replace foutje
~~We hebben een type Page en Category uit theme-wizard-templates, en die werd geimporteerd in theme-wizard-app. Maar theme-wizard-app importeert ook uit theme-wizard-templates, wat een build cycle error veroorzaakte.~~ ~~Dit heb ik tijdelijk opgelost door de type toe te voegen in theme-wizard-app, waardoor theme-wizard-app niet meer afhankelijk is van de build van theme-wizard-templates.~~ ~~Vraag is of dit zo moet blijven, of een andere richting kiezen, voor bijvoorbeeld een "shared" types package? Betekent wel dat je yet another package hebt en de vraag is: hoevaak gaat dit nog voorkomen?~~ ~~Dit heb ik gedaan, zodat de templates standalone kunnen draaien op de theme-wizard-app url, dus lokaal bijvoorbeeld: http://localhost:9492/templates/my-environment/overview~~ - [x] Typing van `TemplateGroup` in `wizard-preview-picker` - [x] Componenten die in `theme-wizard-app` stonden en enkel in `theme-wizard-templates` werden gebruikt, verplaatst naar `theme-wizard-templates` - [x] `template-heading `vervangen door `clippy-heading` en deze in `packages/clippy-components` geplaatst - [x] In deze PR (anders werd de huidige PR te groot), `template-link` vervangen door `clippy-link` en deze in `packages/clippy-components` geplaatst. [Clippy Link Webcomponent PR](#454)
No description provided.