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 to Font Awesome 5 (Develop) #957

Merged
merged 21 commits into from Jul 17, 2019

Conversation

@amosfolz
Copy link
Contributor

commented Apr 6, 2019

Most of the changes are simply updating the 'fa' tag to the new 'fas' convention.

I do not have a Font Awesome 5 Pro subscription so the bakery command still requires some testing.

#870

@codecov

This comment has been minimized.

Copy link

commented Apr 6, 2019

Codecov Report

Merging #957 into develop will increase coverage by 0.03%.
The diff coverage is 47.36%.

Impacted file tree graph

@@             Coverage Diff             @@
##             develop   #957      +/-   ##
===========================================
+ Coverage      66.96%    67%   +0.03%     
- Complexity      1918   1921       +3     
===========================================
  Files            160    161       +1     
  Lines           6693   6700       +7     
===========================================
+ Hits            4482   4489       +7     
  Misses          2211   2211
Impacted Files Coverage Δ Complexity Δ
app/sprinkles/core/src/Util/CheckEnvironment.php 2.91% <0%> (ø) 41 <0> (ø) ⬇️
...count/src/Database/Migrations/v400/GroupsTable.php 100% <100%> (ø) 3 <0> (ø) ⬇️
...sprinkles/admin/src/Controller/GroupController.php 100% <100%> (ø) 55 <0> (ø) ⬇️
...src/Database/Migrations/v430/UpdateGroupsTable.php 100% <100%> (ø) 3 <3> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66df775...5a3d546. Read the comment docs.

@Silic0nS0ldier
Copy link
Member

left a comment

Few things here that will need to addressed from what I can see, also this will be a breaking change so release wise it'll be most likely part of 4.3.

@Silic0nS0ldier Silic0nS0ldier added this to the 4.3.0 milestone Apr 6, 2019

@Silic0nS0ldier Silic0nS0ldier changed the base branch from hotfix to develop Apr 6, 2019

@lcharette

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

Definitively something for 4.3.0 if it may break existing sprinkles.

{
if ($this->schema->hasTable('groups')) {
$this->schema->table('groups', function (Blueprint $table) {
$table->string('icon', 100)->default('NULL')->change();

This comment has been minimized.

Copy link
@lcharette

lcharette Apr 7, 2019

Member

Should it be null, or an empty string? 🤔

This comment has been minimized.

Copy link
@amosfolz

amosfolz Apr 7, 2019

Author Contributor

I went with NULL based off this post: https://stackoverflow.com/questions/38351498/remove-default-in-migration

I also compared to the other columns in the table that did not have a default set when the migration ran, and they all showed NULL for the Default property. In mysql here is how the results of my testing:
image

This comment has been minimized.

Copy link
@Silic0nS0ldier

Silic0nS0ldier Apr 8, 2019

Member

Should be possible to do an export of the create table logic to see if the default value was truly reversed (the cleaner the better, leftovers attract edge cases).

Assuming I remember, I'll take a look at what the DDL ends up being.

This comment has been minimized.

Copy link
@amosfolz

amosfolz Apr 22, 2019

Author Contributor

Here is how "default" migration from \UserFrosting\Sprinkle\Account\Database\Migrations\v400\GroupsTable.php looks
userfrosting groups default

Here is the result of proposed UpgradeGroupsTable.php migration up
UpdatedUP

Here is the result of proposed UpgradeGroupsTable.php migration down
updatedDOWN

This comment has been minimized.

Copy link
@Silic0nS0ldier

Silic0nS0ldier Apr 23, 2019

Member

I think the posted DDL is wrong. Or its mislabeled? I'll still have to look at this myself, but it works as a good reminder none-the-less, so thanks.

This comment has been minimized.

Copy link
@amosfolz

amosfolz Apr 24, 2019

Author Contributor

I am curious about what is wrong?

Other than ('fas fa-user') in this line, the down method returns the table to the 'original' state. I was not certain how to handle this but I thought it would be better to set the default to use the new fas convention rather than the fa.

I relabeled my pictures above that might help clear up any confusion.

amosfolz added some commits Apr 22, 2019

@amosfolz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

@Silic0nS0ldier What can I do to help make this review easier for you?

Also, if it will help simplify things I can move the bakery command into a new branch and out of this PR.

@Silic0nS0ldier

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

@amosfolz My main limitation at the moment is time, or more specifically whenever I have free time there is something jumping up and down screaming for my attention. There isn't much that can be done about that in the meantime unfortunately, however I will get to this (eventually).

One thing that would help however is if you could note that a given PR is ready for review. If I see added commits without a message once the work is done, I'll just end up assuming that isn't in a ready for final review state.

@amosfolz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2019

This is ready for review! :-)

@amosfolz amosfolz changed the title Fa5 Update to Font Awesome 5 (Develop) Jul 12, 2019

@lcharette
Copy link
Member

left a comment

Found two places where the icons are missing :
Capture d’écran, le 2019-07-13 à 17 09 31
Capture d’écran, le 2019-07-13 à 17 09 38

I also get this error. Might be a conflict from the AdminLTE update?

[Error] SyntaxError: Unexpected token ';'. Expected ')' to end an argument list.
	(fonction anonyme) (AdminLTE.js:687)
[Error] TypeError: undefined is not an object (evaluating '$.AdminLTE')
	Code général (AdminLTE-custom.js:24)

And pro doesn't work :(

@amosfolz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2019

Looks like 5.9 was released in June. Might as well update to the newest version while this is a work in progress? https://fontawesome.com/changelog/latest

@amosfolz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2019

Not sure what happened with AdminLTE.js but I am going to blame the linter on my office computer for screwing up that file somehow... The error is fixed in b5d8060.

Also needed to fix the following two icons:
fa-gear no longer exists, so using fa-cog as alternative.

fa-trash-o is now fa-trash-alt. Fixed these in 4ae024b

@lcharette lcharette merged commit d54c6a0 into userfrosting:develop Jul 17, 2019

3 of 4 checks passed

codecov/patch 47.36% of diff hit (target 66.96%)
Details
codecov/project 67% (+0.03%) compared to 66df775
Details
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@amosfolz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

And pro doesn't work :(

Just leaving a note for myself if/when I come back to try to add in the bakery command...
I think the reason it didn't work is that you still need to manually update the asset-bundle with the pro file path and that was something that can't easily be done through bakery.

@amosfolz amosfolz deleted the amosfolz:fa5 branch Jul 17, 2019

@lcharette

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

FA Pro is probably something we can live with having only a guide in the doc on how to do it in your sprinkle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.