search

conversionxl / aybolit

Lightweight web components library built with LitElement.
https://conversionxl.github.io/aybolit/
MIT License
7 stars 8 forks source link

anoblet/feat/vaadin-icon #237

Closed anoblet closed 1 year ago

anoblet commented 2 years ago

This is a large PR with plenty of room for error. I've tested as much as I can, though please review.

The most notable change was the use of getIconNames which isn't present in vaadin-icon and had to be brought into icon-preview.js

https://github.com/conversionxl/aybolit/blob/a5d7ec9bc154c1e314272386af96ae9ef0baa122/packages/storybook/cxl-lumo-styles/icons-preview.js#L109

https://github.com/PolymerElements/iron-iconset-svg/blob/c575f7f30be3174607a76ccf15c16ce4c2416f4c/iron-iconset-svg.js#L102

https://github.com/vaadin/web-components/blob/master/packages/icon/src/vaadin-iconset.js

github-actions[bot] commented 2 years ago

size-limit report πŸ“¦

Path Size
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js, packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js, packages/cxl-ui/pkg/dist-web/cxl-ui.js 65.58 KB (-0.18% πŸ”½)
kylebrodeur commented 2 years ago

Task linked: CU-344efq1 Update /Β Replace with

saas786 commented 2 years ago

@anoblet

did you explore the possibility of removing iron-icon bloat (as its no longer a deps), from packages?

I can see it:

Ξ» npm ls @polymer/iron-icon
aybolit-workspace@ D:\source\repos\github\cxl\aybolit
β”œβ”€β”¬ @conversionxl/cxl-lumo-styles@1.2.0 -> .\packages\cxl-lumo-styles
β”‚ └─┬ @vaadin/vaadin-lumo-styles@23.2.8
β”‚   └── @polymer/iron-icon@3.0.1
└─┬ @conversionxl/cxl-ui@1.2.0 -> .\packages\cxl-ui
  └─┬ @cwmr/iron-star-rating@3.1.4 invalid: "github:conversionxl/iron-star-rating" from packages/cxl-ui
    β”œβ”€β”€ @polymer/iron-icon@3.0.1 deduped
    └─┬ @polymer/iron-icons@3.0.1
      └── @polymer/iron-icon@3.0.1 deduped

ref:

We can defer it now, and remove it later (as seems like v24 will remove it):

ref:

Why we still have @cwmr/iron-star-rating as deps? Since https://github.com/conversionxl/aybolit/pull/228 we now have our own custom star icon?

As it also bring in @polymer/iron-icon.

ref:

I think we should wait for:

https://vaadin.com/roadmap https://vaadin.com/blog/jakarta-ee-is-becoming-mainstream-get-ready-for-spring-boot-3-and-vaadin-24

Looks like a March 2023 release date.

anoblet commented 2 years ago

@saas786

Why we still have @cwmr/iron-star-rating as deps? Since https://github.com/conversionxl/aybolit/pull/228 we now have our own custom star icon?

I agree with you, this should have been removed in #228 . I will create a PR to remove it.

I think we should wait for: https://vaadin.com/roadmap https://vaadin.com/blog/jakarta-ee-is-becoming-mainstream-get-ready-for-spring-boot-3-and-vaadin-24 Looks like a March 2023 release date.

There's no up/down side to merging this PR now. It feels like we're kicking the can down the road though :) From a DX standpoint, someone might be more inclined to use iron-icon without it.

saas786 commented 2 years ago

@saas786

Why we still have @cwmr/iron-star-rating as deps? Since #228 we now have our own custom star icon?

I agree with you, this should have been removed in #228 . I will create a PR to remove it.

Ok, will approve: https://github.com/conversionxl/aybolit/pull/240

I think we should wait for: https://vaadin.com/roadmap https://vaadin.com/blog/jakarta-ee-is-becoming-mainstream-get-ready-for-spring-boot-3-and-vaadin-24 Looks like a March 2023 release date.

There's no up/down side to merging this PR now. It feels like we're kicking the can down the road though :) From a DX standpoint, someone might be more inclined to use iron-icon without it.

No actual benefit to use it now, instead will need testing and search & replace chore on production website. So deferring is better choice. If I didn't saw the v24 alpha release, I would have inclined to merge this, but seeing that, lets put it on hold.