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

Subdomain-specific website themes #355

Merged
merged 11 commits into from
Jan 8, 2022

Conversation

ddabble
Copy link
Member

@ddabble ddabble commented Apr 23, 2021

Depends on #354, and should change base to dev after that PR has been merged.

Highlights:

  • Favicons for main, internal and admin subdomains (68d9559, bd5bea7)
  • Added override of Django admin's base_site.html (acc1664)
  • Added support for interpolating static files (1510bd8)
  • Renamed some of the logo images (382022c)

`get_relative_static` tokens in files whose names end with `.interpolated` are replaced with the path of another static file.
In addition to adding unique favicons for the `internal` and `admin` subdomains, this also improves how the site is displayed in various contexts, like as app (shortcut) icons on iOS, Android and Windows 8/10, and on the MacBook touch bar.

All of the files within the `favicons` folders were generated at https://realfavicongenerator.net/.

(Added the comments in `style.css`, to facilitate searching through the codebase for those hex colors, which appear in multiple favicon-related files.)
@codecov
Copy link

codecov bot commented Apr 23, 2021

Codecov Report

Merging #355 (699f13a) into dev (36be120) will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #355      +/-   ##
==========================================
+ Coverage   82.72%   82.84%   +0.12%     
==========================================
  Files         117      118       +1     
  Lines        4161     4192      +31     
==========================================
+ Hits         3442     3473      +31     
  Misses        719      719              
Impacted Files Coverage Δ
web/static.py 100.00% <100.00%> (ø)

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 36be120...699f13a. Read the comment docs.

@ddabble ddabble mentioned this pull request Apr 23, 2021
* Added an SVG favicon for each of the subdomains
* Reordered the <link> tags in `web/favicon.html`, so that the're sorted from lower to higher quality (when multiple icons are appropriate,
  the browser selects the last one; see https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types in the row for "icon")
* Regenerated the contents of the `favicons` folders (using https://realfavicongenerator.net/), as some of the colors in the previously generated
  logos were slightly off compared to the colors in MAKE's graphic charter
* Replaced `rel="shortcut icon"` with `rel="icon"`, as the `shortcut` link type is deprecated (see the same link as above)
Base automatically changed from feature/news-abstraction to dev November 15, 2021 17:40
@ddabble ddabble marked this pull request as ready for review November 15, 2021 17:41
…abstraction-and-styling

# Conflicts:
#	locale/nb/LC_MESSAGES/django.mo
#	locale/nb/LC_MESSAGES/django.po
Copy link
Member

@sigridge sigridge left a comment

Choose a reason for hiding this comment

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

I dissagree with the favicons, the backgrounds makes it more difficult to see the logo, especially the main one (the one with white background).

One solution (that i would suggest) is to have the same favicon for all subdomains (a transparent one).

Another solution is to have the main one be transparent and the rest of the subdomains as the different colored ones

This was done after feedback on the favicons being too difficult to discern from the background.

Also made the corners of the favicons rounded (50 pt radius - compared to a width/height of 200 pt).
@ddabble
Copy link
Member Author

ddabble commented Jan 7, 2022

I dissagree with the favicons, the backgrounds makes it more difficult to see the logo, especially the main one (the one with white background).

That's fair 😅 I added the changes we discussed in 51f6c84 🙂

One solution (that i would suggest) is to have the same favicon for all subdomains (a transparent one).

Another solution is to have the main one be transparent and the rest of the subdomains as the different colored ones

Just to reiterate what came out of our discussion, having a transparent favicon should ideally be avoided, as the logo becomes hard to see if the tabs of a user's browser has e.g. a light/yellow background color (at least as long as the favicon doesn't have its own border to make it easier to discern the logo from any background color), and so the solution added in the commit mentioned above seems better.

@sigtheidiot Could you try seeing how the favicons look now? 😊

@ddabble
Copy link
Member Author

ddabble commented Jan 7, 2022

Btw, the tests should succeed after #400 has been merged 🙂

Copy link
Member

@Maisergodt Maisergodt left a comment

Choose a reason for hiding this comment

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

I liked that the favicons differ depending on which domain you are visiting! (although you forgot the docs domain but I don't mind since nobody uses it)

Definitely prefer the new icons with rounded corners :)

web/static/web/css/style.css Show resolved Hide resolved
@ddabble
Copy link
Member Author

ddabble commented Jan 7, 2022

(although you forgot the docs domain but I don't mind since nobody uses it)

Yeah, I left it out, since the subdomain is a work in progress (according to the last developer who was working on it), and because I kinda couldn't be bothered finding/making yet another variation of the logo 😅 Also, I thought of the docs as kind of being part of the main site, since they're both public - unlike the internal and admin domains, which both require special permissions to visit 🙂

Copy link
Member

@sigridge sigridge left a comment

Choose a reason for hiding this comment

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

I havent had the time to check the favicons in the browser, but the images look good!

@ddabble
Copy link
Member Author

ddabble commented Jan 7, 2022

@sigtheidiot Alright, I can wait for you to have time to check it out, before merging, if you want 😊

@sigridge
Copy link
Member

sigridge commented Jan 8, 2022

@ddabble Checked them out in browser now. The rounded corners really make a difference and I think they look really good ❤️

@ddabble ddabble merged commit 80e1340 into dev Jan 8, 2022
@ddabble ddabble deleted the feature/base-template-abstraction-and-styling branch January 8, 2022 14:50
@ddabble ddabble mentioned this pull request Jan 8, 2022
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.

3 participants