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

Make editor logo changeable #1336

Merged
merged 8 commits into from
Jul 1, 2024
Merged

Make editor logo changeable #1336

merged 8 commits into from
Jul 1, 2024

Conversation

ciegler
Copy link
Contributor

@ciegler ciegler commented May 8, 2024

Makes the editor logo changeable similar to the studio logo.

@lkiesow
Copy link
Member

lkiesow commented May 8, 2024

I'm not sure if we really want this. I think it's a good thing for Opencast as a project to keep the branding intact and therefor keep the logo in the tool. Any particular reason to replace this?

Discussed this at the technical meeting

@KatrinIhler
Copy link
Member

From the Dev meeting: There were general thoughts on rebranding, but nobody was strongly against this specific pull request.

@lkiesow
Copy link
Member

lkiesow commented May 14, 2024

You added the logo to somewhere else, but it's also still at the old location, isn't it? Can we move that instead?

@ciegler
Copy link
Contributor Author

ciegler commented May 15, 2024

Yes, I will do that. I also saw that I cannot add a label to the PR and it looks like a proper label is required. Is there a way in which I can add a label?

@Arnei Arnei added the type:enhancement New feature or request label May 15, 2024
@majosch
Copy link

majosch commented May 19, 2024

As member of the institution (University of Vienna) that requested this feature I just wanted to add some notes about the motivation - I couldn't speak up in technical meeting because I was in a crowded tram with headphones only.

Personally I'm no big fan of relabeling, because I think it is misleading for users, causes unnecessary overhead and you're running in inconsistencies. But it's not my call.

On the other hand I'm running three instances with multiple tenants and I use different color schemes/logos to be able to identify the actual instance/tenant at a glance.
Doing this on the webserver is the most convenient way.

@marwyg
Copy link
Member

marwyg commented Jun 11, 2024

I build this version locally and it seems to work:
grafik

I changed the window size (horizontally and via zoom) and I switched between all light/dark modes, everything seem to work (it also fixes a bug, where the logo changes it size when you switch between modes). The code also looks fine to me.

Copy link
Member

@Arnei Arnei left a comment

Choose a reason for hiding this comment

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

Please fix the checkstyle issues.

This changes the logo CSS. The logo now looks slightly different from before, making it less similar to Studio than before? The logo is shifted slightly to the right, and has less contrast in dark mode.
Does fix the logo size in dark mode, as Martin mentioned. Never noticed that o.O

@marwyg
Copy link
Member

marwyg commented Jun 11, 2024

I also noticed, that the logo is styled slightly different, but I think, these changes are intentionally.

Copy link

This pull request is deployed at test.editor.opencast.org/1336/2024-06-12_10-59-44/ .
It might take a few minutes for it to become available.

@ciegler
Copy link
Contributor Author

ciegler commented Jun 12, 2024

Thanks for the comments, since this was the first project I was working on I completely forgot to check the Logo's appearances in different Themes. The contrast in darkmode is now similar to the contrast in Studio and the slightly shift is also gone.

Copy link

This pull request is deployed at test.editor.opencast.org/1336/2024-06-12_11-06-19/ .
It might take a few minutes for it to become available.

Copy link
Member

@Arnei Arnei left a comment

Choose a reason for hiding this comment

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

Vite still reports three checkstyle issues.

   96:1  warning  Expected indentation of 4 spaces but found 6  indent
  106:1  warning  Expected indentation of 4 spaces but found 6  indent
  107:4  warning  Missing semicolon                             semi

The CSS looks good to me now. Since Martin said this works, I believe this can be merged once the checkstyle stuff is out of the way.

Copy link

github-actions bot commented Jul 1, 2024

This pull request is deployed at test.editor.opencast.org/1336/2024-07-01_06-58-35/ .
It might take a few minutes for it to become available.

Copy link

github-actions bot commented Jul 1, 2024

This pull request is deployed at test.editor.opencast.org/1336/2024-07-01_07-11-17/ .
It might take a few minutes for it to become available.

@Arnei Arnei merged commit b7c7430 into opencast:main Jul 1, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants