Skip to content

fix(alias): Convert the two-words aliases into one-word aliases #115

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 20, 2021
Merged

fix(alias): Convert the two-words aliases into one-word aliases #115

merged 2 commits into from
Aug 20, 2021

Conversation

ko2in
Copy link
Member

@ko2in ko2in commented Aug 10, 2020

Convert the two-words aliases line graph into linegraph which conflicts with other FUI components which uses the CSS class name line.

Closes

fomantic/Fomantic-UI#1619

@ko2in
fix(alias): Convert the two-words aliases into one-word aliases
f3a8f58 
Convert the two-words aliases `line graph` into `linegraph` which
conflicts with other FUI components which uses the CSS class name
`line`.
@ko2in
Copy link
Member Author

ko2in commented Aug 10, 2020

@fomantic/maintainers I couldn't find the alias for chart line in lib/static/aliases.json and src/static/aliases.json files.

I'm not sure how the alias chart line is generated. Any hint?

lubber-de
lubber-de reviewed Aug 10, 2020
View reviewed changes
Copy link
Member

@lubber-de lubber-de left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This will still not fix the original chart-line which is part of
https://raw.githubusercontent.com/FortAwesome/Font-Awesome/master/metadata/icons.json
where this script is taking the icon names from.
image

@ko2in
fix(alias): Correct the alias chart line to chartline
b58128a 
Correct the alias `chart line` from FontAwesome to be used as
`chartline` in FUI which prevent the CSS classname confliction.
@ko2in
Copy link
Member Author

ko2in commented Aug 10, 2020

@lubber-de I've added the correction for alias chart-line. Please review again.

lubber-de
lubber-de approved these changes Aug 10, 2020
View reviewed changes
Copy link
Member

@lubber-de lubber-de left a comment

Choose a reason for hiding this comment

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

LGTM

@lubber-de lubber-de added the bug Something isn't working label Aug 10, 2020
@prudho
Copy link

prudho commented Aug 11, 2020

Quick question here: will the name of the icon class be chart line or chartline ? If it's the second answer, we might tag this PR as a breaking change since existing code will not work (happen in my app).

@lubber-de
Copy link
Member

@prudho Yes, it will be chartline... 🤔 I already merged the changes in the base repo. I didn't thought of such an impact.
However, you are right, it's a breaking change.
As this repo here is only for creating a complete new set of FontAwesome, i suggest to add another PR to the main repo to add the old classes (rather than reverting the changes) until we recreate a complete set again by this script.

So in the fomantic repo we would have chart.line as well as chartline inside the 2.8.x branch.

Agree?

prudho
prudho approved these changes Aug 11, 2020
View reviewed changes
Copy link

@prudho prudho left a comment

Choose a reason for hiding this comment

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

Yup, it's okay for me. This might be a breaking change, but it's a rather small one, and for my app it doesn't really matter (just visually)... And like you said, we just have to patch the main repo to make the change transparent for 2.8.

@lubber-de lubber-de mentioned this pull request Aug 11, 2020
Merged
@lubber-de
Copy link
Member

lubber-de commented Aug 11, 2020 •
edited
Loading

@prudho
Created a PR accordingly here fomantic/Fomantic-UI#1637

lubber-de added a commit to fomantic/Fomantic-UI that referenced this pull request Aug 11, 2020
@lubber-de
feat(icon): add the old two classes for linegraph and chartline for c…
7445821 
…ompatibility

This PR re-adds the two word icon names for chartline and linegraph as of #1636 to avoid being a breaking change as @prudho mentioned here .

The next full font awesome update will happen in 2.9.0 using the fixes from fomantic/create-fomantic-icons#115 Then two word aliases will be completely removed
@lubber-de
Copy link
Member

Merging now, as the next Fomantic Release which may gets a new FA Update will be 2.9.0

@lubber-de lubber-de merged commit f6776c7 into fomantic:develop Aug 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@exoego exoego exoego approved these changes

@prudho prudho prudho approved these changes

@y0hami y0hami y0hami approved these changes

@lubber-de lubber-de lubber-de approved these changes

@ColinFrick ColinFrick Awaiting requested review from ColinFrick

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ko2in @prudho @lubber-de @exoego @y0hami