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

Update LUCIDE_ICON_LIST with newest Icon names #2891

Merged
merged 2 commits into from Mar 25, 2024

Conversation

luccavp12
Copy link
Contributor

Attempt to fix #2882

Compared to this newest version of Lucide, there have been some icon name changes. For example, a previous version using rx.icon("activity_square") will no longer work and need to be changed to rx.icon("square_activity").

Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!

The main issue I see with this (and apparent from the CI failures) is that these name changes will break existing code that already works...

If possible I think we should maintain a mapping of the old names to the new names so that we can warn users about the removed names and hopefully automatically map them to the new names.

Maybe something like

RENAMED_ICONS_05 = {
    "activity_square": "square_activity",
    "alert_circle": "circle_alert",
    "alert_octagon": "octagon_alert",
    ...
}

def map_deprecated_icon_names_05(tag: str) -> str:
    new_tag = RENAMED_ICONS_05.get(tag)
    if new_tag is not None:
        console.deprecate(
            feature_name=f"icon {tag}",
            reason=f"it was renamed upstream. Use {new_tag} instead.",
            deprecation_version="0.4.6",
            removal_version="0.5.0",
        )
        return new_tag
    return tag

Then in the create method, we can call this function to check if the name was changed.

@Lendemor curious what you think about this approach; lucide seems kind of brazen about breaking things at the moment.

@luccavp12
Copy link
Contributor Author

That makes a ton of sense, thank you @masenf!

Could I have ran the reflex-web CI tests locally before pushing?

Also, what do you think we should do with regards to the names that have been completely dropped by Lucide? I guess we just have to find the closest looking replacement.

@masenf
Copy link
Collaborator

masenf commented Mar 21, 2024

You can check out reflex-dev/reflex-web and install the requirements.txt in a new venv and then install your local version of reflex as well. Then reflex init and reflex run --env prod and you would simulate the CI run.

@masenf
Copy link
Collaborator

masenf commented Mar 21, 2024

As for the ones that are totally removed... if there's a close enough one, that's ideal, otherwise we can just fail.

@Lendemor
Copy link
Collaborator

Lendemor commented Mar 21, 2024

I quite like with the idea of mapping names, anything that prevent existing apps from breaking is good, and failing is a "last resort".

Also another thing, but even if we pin the lucide version to keep a "stable" version, most people will go pickup the tags from lucide home page, so there will be discrepancy between what we accept and what's offered on lucide.dev

@masenf
Copy link
Collaborator

masenf commented Mar 21, 2024

most people will go pickup the tags from lucide home page, so there will be discrepancy between what we accept and what's offered on lucide.dev

I actually hit this myself quite often. I think the lucide library is still getting stable.

@luccavp12
Copy link
Contributor Author

Just added in the suggested mapping function.

@picklelo picklelo merged commit fbc6e7e into reflex-dev:main Mar 25, 2024
62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix some of the lucide icons allowed names
4 participants