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

Add logo and banner #277

Merged
merged 12 commits into from
Jan 23, 2024
Merged

Add logo and banner #277

merged 12 commits into from
Jan 23, 2024

Conversation

matt-graham
Copy link
Collaborator

Following on from #144, adds SVG files for logo we decided on in meeting today to a new images subdirectory plus a corresponding social banner to address task in #246. The text colour on the banner should be visible on both dark and light backgrounds.

I've also configured the Jekyll site to show the logo in the header bar - by default this was very small so I've added some custom (S)CSS to adjust the header div size and center the logo.

Currently only the logo is shown in the header not any text - not sure if it is possible to override this?

docs/_config.yml Outdated
@@ -6,3 +6,5 @@ url: https://UCL-ARC.github.io/python-tooling

aux_links:
Source repository: https://github.com/UCL-ARC/python-tooling

logo: "https://raw.githubusercontent.com/UCL-ARC/python-tooling/main/images/logo.png"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't get a relative path to work locally so used full URL - not sure if we could put images somewhere under docs directory to get this working and/or if there are relevant Jekyll / theme configuration options.

Copy link
Member

Choose a reason for hiding this comment

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

Possibly the images are not included in the artifact upload step (therefore not included in the website deployment??).

I'd say this way is probably fine.

Copy link
Member

Choose a reason for hiding this comment

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

I think you have to do this kind of thing for relative links https://github.com/paddyroddy/paddyroddy.github.io/blob/347e094cb0af0c609394b2beaa90770929feca1e/running.html#L14.

I think your approach is fine, though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks useful to know for future reference!

Comment on lines +2 to +3
max-height: 15em;
height: 15em;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As max-height attribute is set in theme styling found I needed to override both max-height and height for this to work but someone with more CSS knowledge may know a better approach.

images/logo.png Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could potentially remove this. I initially thought it might be necessary to have a raster image for including in theme but rendering SVG directly works. It can be useful to have a PNG for some use cases but easy enough to just generate from SVG.

This reverts commit 2ea85c9.

Removing logo from README for now in favour of adding in separate PR.
Copy link
Member

Choose a reason for hiding this comment

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

Should it go at the top of the README? We can replace the title with the banner, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Having the logo in the README seems a good idea though from a digital accessibility standpoint might be better to have title still as text (though I guess alt text would somewhat solve that). Could have logo above header / title - something like https://github.com/matt-graham/symnum?

Copy link
Member

Choose a reason for hiding this comment

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

Could have logo above header / title - something like matt-graham/symnum?

That looks nice

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've had a go at replicating this in the README - unfortunately to get image to be centre aligned need to fallback to HTML rather than Markdown. I've also centred the heading at the moment but could keep this left aligned and just centre logo. As the image URL includes the branch and the logo is not currently on main (and I guess we don't want to keep this branch around) this won't currently render correctly until merged.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh and the image not existing yet is causing the link checker to fail. I guess probably worth separating out in a different PR - will revert here.

Copy link
Member

@paddyroddy paddyroddy left a comment

Choose a reason for hiding this comment

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

Yay 🫐

@samcunliffe
Copy link
Member

🫐

@matt-graham
Copy link
Collaborator Author

I made an aborted attempt to include a favicon version of logo as noticed Jekyll complained about not being able to find favicon.ico when serving but this didn't seem to work when serving locally so have removed. Also in the course of testing locally I ended up switching the branch in the URL to mmg/add-logo-and-banner but have now reverted that back to main. Given it looks like including in the README here will be a bit 🐔 / 🥚 due to the link checker / image URL not existing yet, I think we can do in a separate PR. @paddyroddy / @samcunliffe just to confirm given I've made several changes since you reviewed, are we okay to merge this?

@paddyroddy
Copy link
Member

I'm happy, merging

@paddyroddy paddyroddy merged commit d650dc1 into main Jan 23, 2024
15 checks passed
@paddyroddy paddyroddy deleted the mmg/add-logo-and-banner branch January 23, 2024 17:17
dstansby pushed a commit that referenced this pull request Jan 23, 2024
Follow on from #277 - adds logo to README file. Hopefully link checker
will now not fail as logo file now on `main` 🤞.
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