5.1 updates #20

mlwilkerson · 2018-03-23T23:51:23Z

- Add FaLayersCounterComponent to use the new counter() API - Remove counter support from FaLayersTextComponent - Update all package dependencies and names for FA 5.1

- Make title optional for FaIconComponent - Add title to an icon in the demo app to exercise this case

mlwilkerson · 2018-03-24T00:17:25Z

@ZeevKatz Could you have a look at this? I'm basically just building off of your structure to add the counter component.

(I can't seem to invite you to a code review for some reason, but maybe you can jump in here anyway).

There are a few questions that came up for me while I was doing this, since it got me to interacting more closely with your work on PR #14.

What is the significance of the cssClass with @HostBinding on each component? Why do we need that?

I left it for now, but it wasn't clear what purpose it serves. I see that it's binding a class name to the angular host component, but not sure what that might be for, when it's essentially duplicating the class name on the <svg>s or layers <span>s.

On FaLayersCounter, I continued using [content] as an attribute to bind the counter value. Does that seem like a good approach? Or now that this counter is its own component, is there a better or more natural way to do this? (i.e. this component's API doesn't have to be an adaptation of the FaLayersTextComponent any more—it can be whatever it wants to be.)
For @robmadole, especially: I thought using the counter() API would eliminate the need for the extra CSS adjustments in layers.component.scss, but it doesn't...yet. Furthermore (I haven't taken time to troubleshoot this yet), but if you look at the demo app, you have not only the red counter over the bell icon where you'd expect it to be, but you also have a really tiny counter way up and to the right of the viewport. Clearly, we have a CSS problem here, but I haven't tracked it down yet.

ZeevKatz · 2018-03-24T17:35:22Z

@mlwilkerson Looks great! Now, FaLayersCounter is pretty same as FaLayersText, so, you should to create a base component to extending those components and prevent duplicating code.
And yes about your number 2, it’s a good approach. FaLayersCounter should be always inside FaLayers content without depending on FaLayersText.

About the cssClass, you need this to enable users to select our components with a class selector and not just with an element selector.
For example, if some users wants to make an absolute positioning icon, they’ll wants to select with .ng-fa-icon selector and not with the host element name.

mlwilkerson · 2018-03-24T19:21:26Z

Thanks for having a look, @ZeevKatz.

Yes, I'd thought about refactoring, so with your suggestion as a confirmation, I'll plan to do that.
I didn't understand this comment you made:

And I think you still needs to limit the FaLayersCounter scope (I don't see why not)

Could you say that another way?

About cssClass, in every case, the Font Awesome elements are already selectable by class within the Angular component. Adding the cssClass bounding to the host component would make the host Angular component selectable by class as well. Is that the idea? I haven't done much with Angular, so I'm not aware of that being a usage pattern—selecting the host component, rather than the stuff inside it. But if that's a desirable usage pattern, so be it.

Thanks again.

mauricew · 2018-03-24T18:42:36Z

package.json

@@ -95,7 +95,7 @@
    "@angular/core": "^5.0.0",
    "@angular/platform-browser": "^5.0.0",
    "@angular/platform-browser-dynamic": "^5.0.0",
-    "@fortawesome/fontawesome": "^1.1.4",
+    "@fortawesome/fontawesome-svg-core": "^1.2.0",


I just experienced this when trying to pull directly from this branch, but the version matcher here won't work since the hyphenation in the specific npm package version makes it non-semver. The react 5.1 branch seems to have it correct

Thanks for having a look at this @mauricew.

I'm not sure I follow as to your expectation and what you mean that this makes it "non-semver". Could you elaborate?

To match a range it seems to require a strict semver, x.y.z and nothing else at the end. The version currently on npm is 1.2.0-7 but trying to match it with ^1.2.0 or ~1.2.0 won't work. (using npm 5.6)

Oh, I see. I just pushed a change for that, adding the prerelease numbers to the version strings.

I've actually been intentionally leaving those off, so as not to update them every time we version these pre-releases. yarn is what I normally use, and it by default—when it sees that there's not a 1.2.0 with the latest tag, prompts me to select a version. So I quickly pick the latest prerelease without thinking about it. And my reasoning is that—once we do publish the real 1.2.0 tagged as latest, it won't require an update of its version in package.json. So it is semantically versioned, at least in some sense: the meaning is "install the latest compatible version with the latest tag, or if there isn't one, give me a chance to pick a prerelease." But I didn't realize npm wasn't giving the same choose-your-own-prerelease-adventure option—when I tried it, it just choked.

So, sorry, and I'll plan to stick with the explicit prerelease version numbers for now, in the interest of the npm users.

(Though, surely there must be a flag for npm to make it work like yarn in this regard, isn't there?)

ZeevKatz · 2018-03-25T07:13:26Z

@mlwilkerson check my comment again

mlwilkerson · 2018-03-25T13:11:08Z

@ZeevKatz got it. thanks.

common functionality.

robmadole · 2018-03-26T22:05:37Z

@mlwilkerson the counter up in the top corner of the demo is coming from our "When using text layer outside layers component". Even though it warns of that it still renders the counter. I think this is expected behavior. We aren't stopping the rendering just warning about it.

devoto13 · 2018-03-26T22:16:16Z

README.md

+## Installation
+
+```
+$ yarn add @fortawesome/fontawesome-svg-core


It may be a good idea to eliminate this step to simplify installation. This package is also included in the dependencies of the @fortawesome/angular-fontawesome package, so users may end up with two copies of it if versions don't match.

Ah, this needs to be peer dependency. Let me double-check that.

Good catch @devoto13, that would cause big trouble for anyone who tries to use the library.

I would actually expect this to be a normal dependency, so it's less packages for users to install. Do you think it is a common use case to have more than one package depending on @fortawesome/fontawesome-svg-core or why do you want to have it as peer dependency?

We need it to be a peer dependency because of the library. If multiple copies of fontawesome-svg-core exist in node_modules you could end up calling library.add() on a completely separate instance of the library. Then when, for example, the angular-fontawesome component goes to look up icons it's working off of a different instance of the library.

We saw this early on with each of our other components. Directory looks a bit like this:

- node_modules - @fortawesome - fontawesome-svg-core # used if you import { library } in your app - angular-fontawesome - node_modules - @fortawesome fontawesome-svg-core # used by angular-fontawesome when that component uses icon()

Maybe there is a better way to solve this. Some other way to have a Singleton that even multiple installed versions of FA can use. But peer dependencies solved it rather nicely.

I see what you're talking about. Because users need to import from fontawesome-svg-core directly to access the icons library. I agree it makes sense to have it this way for now.

devoto13 · 2018-03-26T22:22:16Z

README.md

+
+The library provides convenient usage in your templates but you have to manage
+the icons separate from your components. This means that if someone
+accidentally removes the icon your component needs your component could break.


your component needs your component could break. -> your component which uses it could break. or something

devoto13 · 2018-03-26T22:31:35Z

README.md

 ```
-<fa-icon [icon]="faSync" [spin]="isSyncAnimated" (click)="isSyncAnimated=!isSyncAnimated"></fa-icon>
+
+You can also import entire styles (but be careful!).


Maybe clarify why - it will significantly increase bundle size.

I'll add some elaboration on that while I'm already editing this thing.

mlwilkerson · 2018-03-26T22:57:02Z

README.md

+### Explicit reference
+
+Not as convenient as using the library but if you believe "explicit is better than
+implicit" then this method if for you.


I'm fixing the "if" vs. "is" typo here.

mlwilkerson · 2018-03-26T23:01:11Z

README.md

 ```
-<fa-icon [icon]="faCircle" transform="shrink-9 right-4" [mask]="faSquare"></fa-icon>
+
+### Using the Font Awesome Library


I was confused when I started reading this section. I initially thought that by "Font Awesome Library" you meant the API library, as in fontawesome-svg-core. I re-read the first part about templates 3 times before I realized you were talking about the library.add() library. I wonder if that's going to trip up anyone else? I guess, without thinking really thinking about it, I've started calling fontawesome-svg-core the "base" or "API" library. And the library.add() thing the "icon library". And I suppose a newbie might also wonder if free-solid-svg-icons isn't a library, but I normally call that an "icon package" or "icon pack".

So I'm calling this out as something I've been taking for granted now made strange, forcing me to maybe see it through a newcomer's eyes.

For the moment, I'm going to push a commit that changes this to "Icon Library". But I'll grant that this its debatable what stuff should be named. So, pushback or whatever.

icons bigger, rather than using font-size.

- Now that the counter() API is being used and we don't have conflicting CSS classes. - We still have a problem with the height / line-height layout of the counter.

mlwilkerson · 2018-03-26T23:54:59Z

There's just this one little nuisance remaining about the CSS on the counter, something about the relationship between the height and line-height, it seems.

The goal here is to not rely on the the CSS tweak that @ZeevKatz needed to add to make it look right. My CSS powers have been exhausted trying to understand why it is that the default framework CSS styles for fa-layers-counter work fine in the examples on fontawesome.com, but when we see the counter over the bell in the example app in this repo, the red area of the counter is too tall, and the counter's text value is aligned at the top of that red region, instead of in the vertical middle.

It would be nice to figure that out and remove that one last remain CSS tweak in here. But if we can't figure it out, I guess it doesn't need to block a release.

robmadole · 2018-03-27T14:36:51Z

@mlwilkerson looking at the counter thing again now

robmadole · 2018-03-27T15:30:28Z

Ok @mlwilkerson got the counters fixed with https://github.com/FortAwesome/fontawesome-buildsystem/pull/267

- Remove remain CSS fix for counters, given that the CSS has been fixed in fontawesome-svg-core.

mlwilkerson added 4 commits March 23, 2018 11:06

Use package names and versions for Font Awesome 5.1.

b5690dd

Updates for Font Awesome 5.1

59bae4c

- Add FaLayersCounterComponent to use the new counter() API - Remove counter support from FaLayersTextComponent - Update all package dependencies and names for FA 5.1

Add title support to FaLayersTextComponent

3199e32

Update title support for FaIconComponent

bfcb0ab

- Make title optional for FaIconComponent - Add title to an icon in the demo app to exercise this case

mlwilkerson requested a review from robmadole March 23, 2018 23:55

mauricew suggested changes Mar 24, 2018

View reviewed changes

Add prerelease numbers to fontawesome package version strings.

3f2f1e0

robmadole and others added 6 commits March 26, 2018 12:20

Couple missed old package names

fade4d4

Fix some TS warnings

c0d7e52

Add Travis config

6c2f534

Refactor text and counter components with a base component to handle

72d6101

common functionality.

README and UPGRADING

ed1a68b

Mention examples

8db4b46

Few small nits

9b4bf66

devoto13 reviewed Mar 26, 2018

View reviewed changes

Move svg-core to peer dep

770f7e5

devoto13 reviewed Mar 26, 2018

View reviewed changes

robmadole added 2 commits March 26, 2018 17:26

Editing fixes

4dd9043

Add svg-core as dev dep so tests run

b8a0b59

devoto13 reviewed Mar 26, 2018

View reviewed changes

Remove unused imports.

b8175ca

mlwilkerson commented Mar 26, 2018

View reviewed changes

Add border example

3e2bc24

mlwilkerson added 3 commits March 26, 2018 16:11

Minor doc tweaks

fca7d03

Switch to using Font Awesome layout classes like fa-4x to make example

2d56109

icons bigger, rather than using font-size.

Remove the CSS workaround for the previous class conflict.

d63a5dd

- Now that the counter() API is being used and we don't have conflicting CSS classes. - We still have a problem with the height / line-height layout of the counter.

Update pre-release dependencies

0cd6ecf

- Remove remain CSS fix for counters, given that the CSS has been fixed in fontawesome-svg-core.

mlwilkerson merged commit 6531823 into development Mar 27, 2018

mlwilkerson deleted the 5.1-updates branch March 27, 2018 22:10

devoto13 mentioned this pull request Mar 27, 2018

Added Travis CI #21

Closed

This was referenced Mar 27, 2018

Add title to FaLayersTextComponent for feature completeness #19

Closed

Need a proper documentation for implementation. #16

Closed

devoto13 mentioned this pull request Feb 29, 2020

Make @fortawesome/fontawesome-svg-core a regular dependency #230

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

5.1 updates #20

5.1 updates #20

mlwilkerson commented Mar 23, 2018 •

edited

Loading

mlwilkerson commented Mar 24, 2018

ZeevKatz commented Mar 24, 2018 •

edited

Loading

mlwilkerson commented Mar 24, 2018

mauricew Mar 24, 2018

mlwilkerson Mar 24, 2018

mauricew Mar 24, 2018

mlwilkerson Mar 24, 2018

ZeevKatz commented Mar 25, 2018

mlwilkerson commented Mar 25, 2018

robmadole commented Mar 26, 2018

devoto13 Mar 26, 2018 •

edited

Loading

robmadole Mar 26, 2018

robmadole Mar 26, 2018

devoto13 Mar 26, 2018

robmadole Mar 27, 2018

devoto13 Mar 27, 2018

devoto13 Mar 26, 2018

robmadole Mar 26, 2018

devoto13 Mar 26, 2018

mlwilkerson Mar 26, 2018

mlwilkerson Mar 26, 2018

mlwilkerson Mar 26, 2018

mlwilkerson commented Mar 26, 2018

robmadole commented Mar 27, 2018

robmadole commented Mar 27, 2018

5.1 updates #20

5.1 updates #20

Conversation

mlwilkerson commented Mar 23, 2018 • edited Loading

mlwilkerson commented Mar 24, 2018

ZeevKatz commented Mar 24, 2018 • edited Loading

mlwilkerson commented Mar 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeevKatz commented Mar 25, 2018

mlwilkerson commented Mar 25, 2018

robmadole commented Mar 26, 2018

devoto13 Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlwilkerson commented Mar 26, 2018

robmadole commented Mar 27, 2018

robmadole commented Mar 27, 2018

mlwilkerson commented Mar 23, 2018 •

edited

Loading

ZeevKatz commented Mar 24, 2018 •

edited

Loading

devoto13 Mar 26, 2018 •

edited

Loading