Conversation
|
|
||
| /** | ||
| * @since 1.0 | ||
| * @since 5.2 |
There was a problem hiding this comment.
THe since indicates since when the component has been available. Pleas revert
| * | ||
| * Icon library for this instance. Overrides the global default. | ||
| * - "material-icons" → Material Icons | ||
| * - "material-icons" → Material Icons |
| }) | ||
| export class SixIcon { | ||
| /** Name of the icon, path to SVG file or a data image */ | ||
| @Prop() src?: string; |
There was a problem hiding this comment.
As I already mentioned in #425 (comment)
Using src as the prop name for the input, in case when we are passing an icon name, makes no sense to me. I would separate the different types, meaning
src: SVG File or Data URLname: Name of the icon => you can introduce this already, but keep the slot
If I put myself in the user's shoes, I find this to be a lot easier to understand.
You can then define a hierarchy, like src > name > slot
There was a problem hiding this comment.
Well, i think it makes more sense than having prop name and prop src... how the users know which one takes priority? ... Just makes things more complicated than they should be. I will change it anyway.
| * - <svg><use/></svg> is better for styling (e.g. fill: red), but slower at rendering. | ||
| * - <img> is better for HTTP caching, but you cannot style the internal SVG elements. | ||
| */ | ||
| @Prop({ reflect: true }) inlineSvg: boolean = false; |
There was a problem hiding this comment.
Is this really needed? What performance hit are talking about?
I would simply default to the <svg><use/></svg> since, as you mention, its better for styling
There was a problem hiding this comment.
As you can see there img is good for HTTP caching... you can in some instances visually see the difference rendering/loading the icons...
There was a problem hiding this comment.
There is however a caveat which you have to put id="img" inside your svg for it to work and properties inside svg have precedence i.e. if you have fill in some vector you cannot override it from outside.
There was a problem hiding this comment.
I am not sure if there is a more elegant way to approach this... this is what i have at the moment.
| if (this.src !== undefined) { | ||
| if (isSvg) { | ||
| if (this.inlineSvg) { | ||
| return ( | ||
| <svg part="svg"> | ||
| <use href={`${this.src}#img`} /> | ||
| </svg> | ||
| ); | ||
| } | ||
| return <img src={this.src} />; | ||
| } | ||
| return this.src; | ||
| } | ||
| return <slot />; |
There was a problem hiding this comment.
if (this.src === undefined) {
return <slot />;
}
if (!isSvg) {
return this.src;
}
if (!this.inlineSvg) {
return <img src={this.src} />;
}
return (
<svg part="svg">
<use href={`${this.src}#img`} />
</svg>
);
There was a problem hiding this comment.
Your solution does not work if you want to have name prop... i will adjust it..
| }; | ||
|
|
||
| private renderContent(isSvg: boolean) { | ||
| if (this.src !== undefined) { |
There was a problem hiding this comment.
So the svg here has precedence over the slot?
There was a problem hiding this comment.
Why not? When you explitly place a path to a SVG you are showing a more clear intention to use svg over anything else.
| <six-icon>sick</six-icon> | ||
| <six-icon>fort</six-icon> | ||
| <six-icon>castle</six-icon> | ||
| <six-icon src="fort"></six-icon> |
There was a problem hiding this comment.
I would add a comment explaining the two possible ways to do this => slot and prop
| } | ||
|
|
||
| /* ========================================================================== | ||
| Size helpers (unchanged API, a bit more explicit) |
There was a problem hiding this comment.
Remove the content inside the brackets
add noScroll prop to six-dropdown.
Co-authored-by: Leupp, Joel <joel.leupp@six-group.com>
* chore: update release pipeline to use trusted publisher * chore: update release pipeline and separate into two jobs * chore: improve condition for release type
* chore: readd missing insider release steps * chore: fix insider workflow to be executed again on push in main branch
…value-accessor new angular value accessor for six-checkbox-group
…setup for regular release (#455) * fix: issue with github pages build * docs: adjusted changelog to reflect changes correctly * chore: remove npm token auth for regular release and switch to node 24 for builds with provenance
* docs: update changelog for v5.2.0 * docs: update process for creating a release
* refactor: playwright test infrastructure - Add Playwright test fixtures with animation control - Add coverage utilities and merge scripts - Configure Playwright in stencil.config.ts - Remove Puppeteer dependency - Remove unused testUtil and animation.scss * test: playwright tests for all components Add comprehensive E2E tests covering: - Functional behavior and state transitions - Screenshot tests for visual regression - Accessibility tests with axe-core - Keyboard navigation following ARIA APG patterns Remove legacy Jest spec tests in favor of Playwright tests. * chore: update CI workflow and build configuration - Update Node.js version to 24 in workflows - Add Cobertura coverage report generation - Fix artifact upload paths - Update prettier configuration * docs: add Playwright testing guide and reorganize documentation - Add comprehensive Playwright testing documentation - Create Contributing section with Getting Started, Architecture, Testing - Add Playwright skill for Claude Code - Use angular-html/angular-ts syntax highlighting - Update README files * ci: parallelize workflow into lint-and-test, e2e-tests, and build jobs
# Conflicts: # docs/changelog.md # package-lock.json
fix: prevent type-to-select for six-select autocomplete
…t of font (#468) Co-authored-by: Leupp, Joel <joel.leupp@six-group.com>
* chore: upgrade dependencies to latest versions Update all project dependencies including Stencil 4.41, Angular 19, and framework output targets. Require Node.js >= 24 for all workspaces. * fix: update framework bindings and migrate to ESLint flat config - Migrate ui-library from .eslintrc.json to flat ESLint config - Fix Angular output target to generate proper component bindings - Update React output target with correct autocorrect boolean type - Fix control value accessors to use proper inject() function - Update examples and documentation to match new API * fix: replace Sass @import with @use to fix deprecation warnings Sass @import rules are deprecated and will be removed in Dart Sass 3.0.0. Update all component SCSS files to use the modern @use syntax. * fix: update pkg-pr-new workflow to correctly publish the angular library * chore: npm audit fixes * ci: update dependency-review version to fix size issue * docs: update changelog and doc files --------- Co-authored-by: colinscz <19342760+colinscz@users.noreply.github.com>
Co-authored-by: GitHub action <>
Co-authored-by: GitHub action <>
…ix-group/six-webcomponents into feat/six-icon-with-svg-support # Conflicts: # libraries/ui-library/src/components/six-icon/test/six-icon.spec.tsx
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 09ed2fc |
|
Please forgive me if I'm not quite seeing through everything ;-) But there seem to be a lot of commits in this PR which have nothing to do with it, like the ones from Béla and Joel. |
| @@ -11,7 +11,7 @@ | |||
| <script type="module"> | |||
| document.addEventListener('DOMContentLoaded', () => { | |||
| // Set Material Symbols as the default for all <six-icon> | |||
| window.six.setDefaultIconLibrary('material-symbols'); | |||
| // window.six.setDefaultIconLibrary('material-symbols'); | |||
There was a problem hiding this comment.
Do we really want to ship an icon called test.svg with the web-components?
| 'wght' 400, | ||
| 'GRAD' 0, | ||
| 'opsz' 24; | ||
| font-variation-settings: 'wght' 400, |
There was a problem hiding this comment.
Is your formatter off or was it off before? I'd prefer it if the formatting stays as it was before, unless there are good reasons to remove the line break?
|
Can you rebase this branch @rspy? I am seeing over 500 changed fiels which makes it a bit difficult to review 😅 |
7 participants
🔗 Linked issue
❓ Type of change
📚 Description
#425
📝 Checklist
mainbranchfix #xxx[,#xxx], where "xxx" is the issue number)If adding a new feature, the PR's description includes:
Other information: