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.x - Update Milligram to 1.4.1, adjust stylings and fix problem with DB connection test #915

Merged
merged 4 commits into from
Oct 8, 2022

Conversation

LordSimal
Copy link
Contributor

This PR contains a few changes 😄

Update Milligram to 1.4.1

Here I just downloaded the latest minified version of the milligram Framework, replaced the file in webroot/css/milligram.min.css and checked if everything still works fine.

Here I noticed, that the current version of milligram doesn't have a column-responsive class anymore.
Therefore I will create a separate PR for cakephp/bake which adjust the bake template to not use that class anymore.

Fix default homepage DB connection test

We changed the signature of Driver::connect() to be void instead of bool because we throw an exception when there is an error trying to connect. Therefore I adjusted the logic to show correctly if the currently set connections are functional or not.

image

Adjust styling for pagination

I took the liberty to adjust the styling of our default paginator:
image
image
image

In my opinion this looks a bit better than what we have currently.

Download Raleway Google Font and include statically

Since I had to do basically this exact thing on dozens of other pages for my company I did the same here.
I basically downloaded all the raleway WOFF2 files, put it in the font folder and statically linked the font-faces to that file.

The main reason here is the fact, that according to GDPR it is not allowed to load Google Fonts via an external URL because the IP address is being sent to Google (of course, otherwise transportation of data won't work) but for some reason this is not allowed without user consent........ ANYWAY

With that out of the way the only external file we are currently loading is the CakePHP logo on the default homepage (which is fine I would say)
image

Put every color inside a CSS var and cleanup cake.css

Since we already introduces CSS variables in the DebugKit CSS/JS rework I have now also added them in here.

The only other thing I can show you is how all the major input types look by default:
image
image

If you think something can be adjusted please tell me 😄

@ADmad
Copy link
Member

ADmad commented Oct 2, 2022

Using local fonts instead of from google is something that should be done for 4.x too. Besides the GDPR issue that would make working locally easier without internet access.

@LordSimal
Copy link
Contributor Author

Using local fonts instead of from google is something that should be done for 4.x too

I can do that in a follow up PR 👍🏻

@ADmad
Copy link
Member

ADmad commented Oct 2, 2022

Based on release notes of 1.4.0 normalize.css is already included in milligram. So could you try without it?

@markstory
Copy link
Member

I basically downloaded all the raleway WOFF2 files, put it in the font folder and statically linked the font-faces to that file.

Is this within the licensing of google fonts? We don't want to be infringing on their licensing. If it isn't we can use standard web-safe fonts instead.

text-align: center;
}
.pagination a {
border: 2px solid var(--color-cakephp-blue);
Copy link
Member

Choose a reason for hiding this comment

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

This feels heavy to me. Do we need a border at all on these they are just links after all.

Copy link
Contributor Author

@LordSimal LordSimal Oct 6, 2022

Choose a reason for hiding this comment

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

It was just a personal style adjustments because I like the bordered pagination links with the hover effect more. But I will remove that styling if you don't like it.

@ADmad
Copy link
Member

ADmad commented Oct 6, 2022

Is this within the licensing of google fonts?

Raleway is licensed under the SIL Open Font License. So I don't think where source them from should matter.

@LordSimal
Copy link
Contributor Author

The Pagination styling has now been reverted to this state:

image

image

image

@garas
Copy link
Member

garas commented Oct 6, 2022

License has this condition:

  1. Original or Modified Versions of the Font Software may be bundled, redistributed and/or sold with any software, provided that each copy contains the above copyright notice and this license. These can be included either as stand-alone text files, human-readable headers or in the appropriate machine-readable metadata fields within text or binary files as long as those fields can be easily viewed by the user.

So it should include license somewhere with font files.

Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@ADmad ADmad merged commit c0feda1 into 5.x Oct 8, 2022
@ADmad ADmad deleted the 5.x-milligram-update branch October 8, 2022 03:13
@berarma
Copy link
Contributor

berarma commented Feb 16, 2023

Milligram 1.4.x doesn't include normalize.css, it's a dependency.

@LordSimal
Copy link
Contributor Author

Then the release notes for 1.4.0 are pretty misleading...
Can you provide a PR which re-adds the latest version of normalize.css again?

@berarma
Copy link
Contributor

berarma commented Feb 16, 2023

Then the release notes for 1.4.0 are pretty misleading... Can you provide a PR which re-adds the latest version of normalize.css again?

I understand it as updating dependencies. I tested it with and without normalize and the only change I noticed was the body margin. That's on my browser but since normalize is used to remove browser differences it could be different in others browsers.

If there's no objections about me being wrong I'll do a PR tomorrow.

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.

5 participants