Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maybe we should replace archived chadweimer/iron-star-rating with chadweimer/mwc-star-rating? #227

Open
saas786 opened this issue Sep 23, 2022 · 9 comments
Assignees
Labels
enhancement New feature or request

Comments

@saas786
Copy link

saas786 commented Sep 23, 2022

Every time I try to install npm packages in wp-theme-lib I have to face this issue.

ConEmu64_L2jtN6q56Q

Plus we maybe able to drop our fork and use upstream as is.

Ref: #92 (comment)

@saas786 saas786 added the enhancement New feature or request label Sep 23, 2022
@anoblet
Copy link
Collaborator

anoblet commented Sep 26, 2022

It looks like we would need to override the template for mwc-star-rating. In order to do so, we would need the author to migrate to the lit package. I have filed an issue: chadweimer/mwc-star-rating#2

@saas786
Copy link
Author

saas786 commented Sep 26, 2022

It looks like we would need to override the template for mwc-star-rating.

Why we need to override the template?

@anoblet
Copy link
Collaborator

anoblet commented Sep 26, 2022

I believe this template needs to be overridden in order to display vaadin:star.

https://github.com/chadweimer/mwc-star-rating/blob/b3843159a267cbe3d1be0378b769360c84dc4048/src/MwcStarRating.ts#L74-L80

@saas786
Copy link
Author

saas786 commented Sep 27, 2022

I believe this template needs to be overridden in order to display vaadin:star.

chadweimer/mwc-star-rating@b384315/src/MwcStarRating.ts#L74-L80

If we file a PR, only than I believe plugin author might opt for lit.

As I see mwc-star-rating uses mwc-icon which is based on v^0.22.0,
ref: https://github.com/chadweimer/mwc-star-rating/blob/b3843159a267cbe3d1be0378b769360c84dc4048/package.json#L21

which also relies on lit-element

ref: https://github.com/material-components/material-web/blob/v0.22.1/packages/icon/mwc-icon.ts#L6

Though latest version uses lit
ref: https://github.com/material-components/material-web/blob/master/icon/lib/icon.ts#L7

@anoblet

Possible to run size check?

  1. With current star rating (might be able to check it when doing #2, size reduction might suggest size diff)
  2. With https://github.com/chadweimer/mwc-star-rating
  3. With #2 but if used latest @material/mwc-icon: 0.27.0

I believe #3 will be the lightest, but as we still need to replace its icon <mwc-icon> with <vaadin:star> and is it worth using it?
Are we also doing any additional logics for this star implementation? Or just replacing the icon?

@anoblet
Copy link
Collaborator

anoblet commented Sep 27, 2022

I filed a PR which migrates to lit (chadweimer/mwc-star-rating#3). mwc-star-rating uses lit-html^1.3.0 which has conflicts when using more than one version of html. lit-html@^2.0.0 got rid of the requirement of having to dedupe those instances.

I have tested extending mwc-star-rating after migrating to lit, and I believe as long as we are able to override the template and the styles to vaadin-icon, we should be good to go.

@saas786
Copy link
Author

saas786 commented Sep 28, 2022

Re-arrange devDependencies as they were before PR, as it shows un-necessary diff.


chrome_Okm3JLkArn


You also need to update @material/mwc-icon to v0.27.0

So we have lit version of mwc-icon too.

ref: https://github.com/material-components/material-web/releases/tag/v0.27.0

@anoblet
Copy link
Collaborator

anoblet commented Oct 3, 2022

@saas786

I cleaned up package.json and upgraded @material/mwc-icon.

@anoblet
Copy link
Collaborator

anoblet commented Oct 11, 2022

I created a PR which copies chadweimer/mwc-star-rating: #228

@lkraav
Copy link

lkraav commented Feb 11, 2023

This should be closed by #228 @anoblet? Please add "fix" / "close" verbs to commit messages in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants