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

Minified CSS icons #322

Closed
prologic opened this issue Aug 20, 2020 · 35 comments
Closed

Minified CSS icons #322

prologic opened this issue Aug 20, 2020 · 35 comments

Comments

@prologic
Copy link

Before:

Screen Shot 2020-08-20 at 17 02 07

After:

Screen Shot 2020-08-20 at 17 02 12

I'm not an expert in CSS to understand what you're tool is doing to my CSS here :/

@prologic
Copy link
Author

@prologic prologic changed the title CSS minify and CSS Icons look weird Minified CSS icons Aug 20, 2020
@tdewolff
Copy link
Owner

tdewolff commented Aug 20, 2020

Which version are you using of minify? Could you try with the latest v2.8.0?

I've ran the following test and the icons show fine for me:

minify -o icss.min.css icss.css
minify -o icons.min.css icons.css
<!doctype html>
<html lang="en">
<head>
  <meta charset="utf-8">
  <title>Test</title>
    <link rel=stylesheet href="icss.min.css">
    <link rel=stylesheet href="icons.min.css">
</head>
<body>
    <i class="icss-text-bold"></i> test
</body>
</html>

Please let me know if you have something so I can reproduce the bug. Thanks!

@prologic
Copy link
Author

I was running the latest from master and I ran the following:

minify -o twtxt.min.css *.css

@prologic
Copy link
Author

Or in other words I was trying to combine and minify all the CSS into a single file.

@tdewolff
Copy link
Owner

I've done the same thing (combine and minify all CSS to the same file) and I still get icons that are working well, I'm unable to reproduce the bug. Could you send the HTML file that is giving this problem so I can check? Can you make sure this is a CSS problem by confirming that the bug exists only after minifying the CSS (and not the HTML or other sources)? I was hoping you could give me an HTML + CSS file that shows the bug so that I can reproduce it.

@prologic
Copy link
Author

The branch is here

git clone github.com/prologic/twtxt
cd twtxt
git checkout minify_css_js_assets
make deps
make

Hopefully this helps! Yes I can confirm the weirdness in the icons is only present after minification/combination.

@prologic
Copy link
Author

Any ideas? I'm not sure my trying again will magically fix things? :)

@tdewolff
Copy link
Owner

I'll get back to you after the weekend

@prologic
Copy link
Author

Can I do anything more to help figure this one out?

@tdewolff
Copy link
Owner

The problem is in the order of minification of files. You need to have icss.css before icons.css, which is in the right order originally due to the order of loading the CSS files, but in the minification this is not the case. Actually, in the current branch I executed the following commands which worked well:

$ cd internal/static/css
$ rm twtxt.min.css 
$ minify -o twtxt.min.css -v *
INFO: infer mimetype from file extensions
INFO: minify to output file twtxt.min.css
INFO: (5.818545ms,  50 kB,  38 kB,  75.5%, 8.6 MB/s) - (01-pico.css + 02-theme-switcher.css + 03-icss.css + 04-icons.css + 99-twtxt.css) to twtxt.min.css
INFO: 6.279787ms total

As you can see, the order is correct now. The order by the way is determined by your bash script (which expands * before calling minify) and not by minify. It might be best to verbosely specify the order to avoid this problem.

@prologic
Copy link
Author

This is what I thought too! So what I did was use an old UNIX trick and rename all the *.css with a numerical NN-` prefix. Hmmm Let me have another stab at this, perhaps it was my fault all along 🧐

@prologic
Copy link
Author

Yup it works! I did something wrong before and/or I was half asleep 🥱

https://github.com/jointwt/twtxt/commit/3f98ae669dd94fc1a25a6f6e788aa6d54c2e6dc6 fixes this for me.

@prologic prologic reopened this Aug 24, 2020
@prologic
Copy link
Author

Oh oops I feel so dumb. I forgot to add originally that I was seeing this issue only on IE11

See attached.

Screen Shot 2020-08-25 at 09 17 19

@tdewolff
Copy link
Owner

Did you properly refresh the browser cache? I think in IE that is a Shift+click on the refresh symbol.

@prologic
Copy link
Author

Oh and one more thing I have noticed one icon in particular that lost its "eyes" -- It was suppose to be a smiley face :D

Screen Shot 2020-08-26 at 7 48 56 AM

@prologic
Copy link
Author

Did you properly refresh the browser cache? I think in IE that is a Shift+click on the refresh symbol.

Yes, even Deleted its cache :D Although I will check with a real user that uses a real Windows PC with IE11 since I was only able to test on a VM.

@prologic
Copy link
Author

Please check/test https://twtxt.net (which you can find the source at https://github.com/jointwt/twtxt)

@tdewolff
Copy link
Owner

The eyes of the smiley is a bug, working on a solution. The IE11 problem is something I can't test on my Linux easily, I'm pretty sure this must be an artifact of the order of the CSS files, but if it persists I'll need to install a virtual machine.

@prologic
Copy link
Author

prologic commented Sep 1, 2020

Thank you! Look forward to the fix, Can we cut a new release too?

@tdewolff
Copy link
Owner

tdewolff commented Sep 1, 2020

I'll push out a new release very soon, what happened with the IE11 problem?

@prologic
Copy link
Author

prologic commented Sep 1, 2020

I just pused jointwt/twtxt@dc008b2dcdde577a824ac922a11dd4b1f17e9ab8 with a new version of the tool by just updating ot master locally. I will still need a new release so the CI/CD pipeline works :) I'll check IE11 to see how it affected the icons there and report back!

@prologic
Copy link
Author

prologic commented Sep 1, 2020

Seems the icons don'y really work very well in IE11 (still) even with the updated tooling.

Screen Shot 2020-09-01 at 22 20 59

@tdewolff
Copy link
Owner

tdewolff commented Sep 1, 2020

Ok I'll take a look with a virtual machine

@tdewolff
Copy link
Owner

tdewolff commented Sep 1, 2020

I see the issue aswell in IE11, it looks like it uses #0000 instead of transparent to display the background-color. This doesn't happen on the latest version of minify and might be related with the fact that make deps is downloading minify v1 instead of v2, you need to use github.com/tdewolff/minify/v2/... to prevent that. Does using v2 or master fix this?

For me the background-color:transparent in i.icss-text-bold gets minified to background-color:initial and not background-color:#0000 as in your case.

@prologic
Copy link
Author

prologic commented Sep 2, 2020

Let me try and report back!

@prologic
Copy link
Author

prologic commented Sep 2, 2020

Hmmm I didn't realise you had a v2 of this tool, I tried to update to it with:

go get -u github.com/tdewolff/minify/v2/cmd/...

But re-generating the bundles produces no differences. Am I missing something?

@prologic
Copy link
Author

prologic commented Sep 2, 2020

Actually I _feel rather stupid here or Go still doesn't support go get package/cmd/... specific version? How do you do this!? 😳

@prologic
Copy link
Author

prologic commented Sep 2, 2020

I just tried to build from a Git checkout and install the tool:

prologic@Jamess-iMac
Wed Sep 02 17:14:27
~/Contributions/minify
 (master) 0
$ git id
da67d20
prologic@Jamess-iMac
Wed Sep 02 17:14:28
~/Contributions/minify
 (master) 0
$ go install ./cmd/minify/...

And in my project:

prologic@Jamess-iMac
Wed Sep 02 17:15:22
~/Projects/jointwt/twtxt
 (master) 0
$ command -v minify
/Users/prologic/go/bin/minify
prologic@Jamess-iMac
Wed Sep 02 17:15:24
~/Projects/jointwt/twtxt
 (master) 0
$ minify --version
minify built from source
prologic@Jamess-iMac
Wed Sep 02 17:15:28
~/Projects/jointwt/twtxt
 (master) 0
$ make generate
prologic@Jamess-iMac
Wed Sep 02 17:15:30
~/Projects/jointwt/twtxt
 (master) 0
$ git status
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   Makefile

no changes added to commit (use "git add" and/or "git commit -a")

Still produces no difference in the generate bundled files. So the IE11 bug remains 🤔

@tdewolff
Copy link
Owner

tdewolff commented Sep 2, 2020

Just to make sure, can you download the binary from GitHub of v2.9.3 (from https://github.com/tdewolff/minify/releases/tag/v2.9.3) and use that binary. minify --version should then give you minify v2.9.3. Using that binary, can you go to internal/static/css in either master or minify_css_js_assets and execute minify -o twtxt.min.css --verbose --bundle 01-pico.css 02-icss.css 03-icons.css 99-twtxt.css. This must give you a different twtxt.min.css where there should be no background-color:#0000.

Note that --bundle is a new option for the binary from v2.9.3 in order to explicitly combine the input files to a single output file.

@prologic
Copy link
Author

Looks like I already have this updated to the tool and minified/combined css bundle:

(⎈ |netdata-production:infra)
prologic@Jamess-iMac
Tue Sep 15 02:40:59
~/Projects/jointwt/twtxt/internal/static/css
 (master) 0
$ /Users/prologic/Downloads/minify -o twtxt.min.css --verbose --bundle 01-pico.css 02-icss.css 03-icons.css 99-twtxt.css
infer mimetype from file extensions
minify to output file twtxt.min.css
(1.378439ms,  56 kB,  42 kB,  75.7%,  41 MB/s) - (01-pico.css + 02-icss.css + 03-icons.css + 99-twtxt.css) to twtxt.min.css
finished in 1.586742ms
(⎈ |netdata-production:infra)
prologic@Jamess-iMac
Tue Sep 15 02:41:11
~/Projects/jointwt/twtxt/internal/static/css
 (master) 0
$ git st .
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean
(⎈ |netdata-production:infra)
prologic@Jamess-iMac
Tue Sep 15 02:41:14
~/Projects/jointwt/twtxt/internal/static/css
 (master) 0
$ sift 'background-color:#0000'

Does this match your expectations?

@tdewolff
Copy link
Owner

Sounds good to me! Does it work well in your browsers?

@prologic
Copy link
Author

Sounds good to me! Does it work well in your browsers?

Yeah no it still looks terrible in IE11 :/ I'm really not sure why :) Did you manage to get an IE11 VM? I got it off MS's developer site here maybe you can see what's going on?

@tdewolff
Copy link
Owner

I did manage to get IE11 in my VM and it worked well. Can you please look at the CSS code for the element that is bad (Inspect Element in IE11)?

@prologic
Copy link
Author

Screen Shot 2020-09-17 at 15 00 15

What am I looking for again? 😀

@prologic
Copy link
Author

Vs Chrome (which is correct):

Screen Shot 2020-09-17 at 15 02 41

Screen Shot 2020-09-17 at 15 02 48

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

No branches or pull requests

2 participants