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

fix(images): add favicon and brand image #280

Merged
merged 6 commits into from
Sep 17, 2021

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Sep 10, 2021

Fixes #279

Depends on #281

image

@jiekang
Copy link
Contributor

jiekang commented Sep 10, 2021

I think for the black background, we can use a version that's white. I can generate one for you. Thoughts?

@jiekang
Copy link
Contributor

jiekang commented Sep 10, 2021

Also I can generate an Icon version that you can add; it's a bit weird to see the full Cryostat logo along with the Cryostat text immediately under it...

@jiekang
Copy link
Contributor

jiekang commented Sep 10, 2021

Oh and I need to make the background for the favicon transparent. Oops

@andrewazores
Copy link
Member Author

Sure, that all sounds good.

@hareetd
Copy link
Contributor

hareetd commented Sep 10, 2021

I like the logo, looks good :)

Also, I understand what the other changed files do but what is webpack.common.js responsible for? How does it fit in with the rest?

@andrewazores
Copy link
Member Author

andrewazores commented Sep 10, 2021

So, the overall build process for a React application typically has a few steps. tsc is the TypeScript compiler, which compiles TypeScript down to plain JavaScript that a browser can execute. This gets invoked by Webpack, which also does various other things for us, and is typically called a "bundler". Webpack's main job is to gather up all the pieces of the app - .js, .html, .css, images/fonts/etc., and assemble it into some kind of usable end format. In our usual build process that's the dist directory, which ends up containing index.html (which is the same index.html from src, but with the favicon link generated and placed into it), various app.bundle.js files containing the TypeScript side of our React components and our various JS dependencies, and again fonts/images/etc. Webpack is also largely responsible for resolving dependencies between files and inserting correctly-generated links between assets so that the end user's browser can properly load them, so for example getting the resulting server URL for the logo asset to load into the About modal is actually done by Webpack and its url-loader. TypeScript/tsc doesn't actually know how to do that, we just have a type definition file telling it that .svg and .png files exist and can be imported, but it has no idea what that would actually mean.

webpack.common.js is the main configuration for Webpack. webpack.prod.js contains additional settings/overrides used for production builds, which is what you get when you do yarn build (and what goes into a finished Cryostat image when you do mvn package). webpack.dev.js contains settings/overrides for the webpack-dev-server (yarn start:dev).

@hareetd
Copy link
Contributor

hareetd commented Sep 10, 2021

Makes sense, thank you!

@andrewazores
Copy link
Member Author

andrewazores commented Sep 13, 2021

Here's the latest update:

image

@jiekang
Copy link
Contributor

jiekang commented Sep 13, 2021

Hm... Okay so atm there's Cryostat icon on top of the text, but the vertical spacing between them is quite large. I think it would be best to replace the both of them with just a Cryostat logo image instead.

@andrewazores
Copy link
Member Author

FWIW that's what the Patternfly example component looks like: https://www.patternfly.org/v4/components/about-modal

I can make it look like this easily enough however:

image

@jiekang
Copy link
Contributor

jiekang commented Sep 13, 2021

Oh... well damn. The vertical space is so off though :|

@jiekang
Copy link
Contributor

jiekang commented Sep 17, 2021

@andrewazores any chance you could share another screenshot of what it looks like now?

@andrewazores
Copy link
Member Author

image

image

jiekang
jiekang previously approved these changes Sep 17, 2021
Copy link
Contributor

@jiekang jiekang left a comment

Choose a reason for hiding this comment

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

One nit:

Is src/app/assets/cryostat-3-icon-white-500px.png file still needed? Rest LGTM.

@andrewazores
Copy link
Member Author

One nit:

Is src/app/assets/cryostat-3-icon-white-500px.png file still needed? Rest LGTM.

It was unused, so I've removed it and rebased. There was just one small conflict with some imports.

@andrewazores andrewazores merged commit 4f42251 into cryostatio:main Sep 17, 2021
@andrewazores andrewazores deleted the image-assets branch September 17, 2021 17:24
mergify bot pushed a commit that referenced this pull request Sep 17, 2021
* fix(images): add favicon and brand image

* fix(license): don't expect license in images

* Update assets

* Use logo variant

* Update logo and use horizontal variant in masthead

* Remove unused asset

(cherry picked from commit 4f42251)

# Conflicts:
#	src/app/AppLayout/AppLayout.tsx
andrewazores added a commit that referenced this pull request Sep 17, 2021
* fix(images): add favicon and brand image (#280)

* fix(images): add favicon and brand image

* fix(license): don't expect license in images

* Update assets

* Use logo variant

* Update logo and use horizontal variant in masthead

* Remove unused asset

(cherry picked from commit 4f42251)

# Conflicts:
#	src/app/AppLayout/AppLayout.tsx

* fixup

Co-authored-by: Andrew Azores <aazores@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing favicon and about modal logo
3 participants