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

#5: Update class names, use scss #6

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

yringler
Copy link

@yringler yringler commented Sep 18, 2019

Assists #5.

I think scss is awesome.
If you don't want it, my master branch doesn't have it, and I can make a new PR.
Note that this PR only updates those classes which you already have to match font-awesome version 5. It does not add new icons.

@yringler
Copy link
Author

To streamline sass, now it has a package.json.
I think it would be great to have this in npm.

@yringler
Copy link
Author

Never mind, I see you already published it.

@fisharebest
Copy link
Owner

Sorry for not replying earlier. I've been very busy on other projects.

You've included several things in one PR.

I'm a server side developer, and have limited experience with the latest front-end tools.
I have to use google every time I want to know the difference between sass and scss...

You've added a yarn.lock file. I'm familiar with npm, but have never used yarn. Scanning the google results for "npm vs yarn" suggests npm is the prefered option. But I don't know enough about the differences to have an opinion.

I look at all the dependencies that you've added for what is a small one-file library.
As a package maintainer, I'm supposed to be aware of all my dependencies, and monitor them for updates, etc.

I'm guessing the target audience for this package is someone (like me?) who wants a particular theme/cms/package to work in RTL, and has enough skill to copy/paste some CSS into a template.

Isn't this just massive overkill?

@fisharebest
Copy link
Owner

BTW - I no longer use this package myself.

These days, I just add a class icon-flip-rtl to any icon that needs reversing.

So if you want to take ownership of this project, I'd be happy to transfer it to you.

@yringler
Copy link
Author

Sorry for not replying earlier. I've been very busy on other projects.

👍 np, thanks for getting back to this.

You've included several things in one PR.

I do have another branch without all the scss stuff etc, but I do prefer scss. I'll get back to that in a second.

I have to use google every time I want to know the difference between sass and scss...

They're the same thing, really. The only difference is whether blocks are defined by brackets or white space.

You've added a yarn.lock file. I'm familiar with npm, but have never used yarn. Scanning the google results for "npm vs yarn" suggests npm is the prefered option. But I don't know enough about the differences to have an opinion.

I work pretty heavily with nodejs, and am pretty familiar with npm and yarn. I started using yarn a few months ago, and can't imagine going back. In my experience/opinion, it's light years ahead in terms of speed and usability, and has some really nifty advanced features (like workspaces).

Npm users could still build this branch; the only difference would be that npm uses npm.lock, so if someone added a package with npm I'd add it again with yarn to make sure the lockfile is up to date.

All that said, for such a tiny project, with 2 source files and 1 dependency, it really doesn't make much of a difference, and I could just drop the lock file.

I look at all the dependencies that you've added for what is a small one-file library.

Only one dependency was added, on dart-sass.

I'm guessing the target audience for this package is someone (like me?) who wants a particular theme/cms/package to work in RTL, and has enough skill to copy/paste some CSS into a template.

For sure, that's a prime target audience. That's why I "bit my tongue" and added the compiled css (generated from the scss) to source control, so that any one who finds this repo can easily copy and paste the css into where ever they want.

Should the README be updated, to point more clearly to the compiled css? I wouldn't want to scare anyone away.

I do want to to point out that currently the .css has "raw" unicode, (there's an open issue sass/dart-sass#568 to toggle that).

But there is another audience. For example, where I work, we use scss with webpack and all that other wonderful nodejs CI stuff, we could use the niceness of scss.

Isn't this just massive overkill?

Well... here's my take;
Scss is huge. Like, beyond huge. I've written some pretty esoteric stuff in the past, so I should know. But using scss doesn't mean that you have to read the whole 1000 page manual. There are some very basic, intuitive features that increase maintainability and extensiblity, while still allowing someone with no scss experiance to modify the source.

For example: Currently all the unicode code points are hard coded. Fontawesome actually defines scss variables for each icon. So instead of writing "\f177", you could write "$fa-var-long-arrow-left".

  1. It's possible to visually see if there's a mistake
  2. If there is a good reason for a very different name, it is a good reminder to write a good comment.

Most importantly for my usage: we identify rtl/ltr with classes on the document body (.dir_rtl, .dir_ltr). So it's nice to be able to change one thing in one place, and have the css use different selectors for direction. Which is possible in scss - I made a function which takes the two selectors as arguments, and uses those variables.

@mixin font-awesome-rtl($ltr-selector, $rtl-selector) {
...
	#{$rtl-selector} .fa-spin {
		animation-direction: reverse;
	}

	#{$ltr-selector} .fa-spin {
		animation-direction: normal;
	}
...
}

Also, in places where you have 2.14286em, you could write the calculation its based on: $fa-li-width * 5/4. (actually, as I was checking this. I noticed that the margin has changed to 2.5 in fontawesome 5. Basing it on the official variable would make this somewhat future proof).

Another useful thing is nesting, where

.parent {
  .child-a { color: black; }
  .child-b {color: blue; }
}

becomes

.parent .child-a { color: black; }
.parent .child-b {color: blue; }

Currently, the start and end styles for a given icon are placed together. If we switch to using variable names, it might make sense to take care of all the rtl styles, and than all the ltr styles, and rely on the variables for clarity. Than we could use file nesting, which would remove a lot of visual noise, which becomes more important if we want to add support for all the new fontawesome icons.

So if you want to take ownership of this project, I'd be happy to transfer it to you.

I dunno. If yes, I think we should agree on future direction before transferring.

TL;DR
Scss usage can be simple, and adds flexibility and stability.

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.

None yet

2 participants