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

Update logos site-wide with new OpenSearch branding #376

Merged
merged 1 commit into from
May 26, 2021

Conversation

tmarkley
Copy link
Contributor

@tmarkley tmarkley commented May 25, 2021

Description

  • Overall, I couldn't figure out a way to import a custom SVG as an EUI iconType and include it with CSS. I resorted to importing the SVG files and using them directly in the EUI components as stated in the documentation.
  • The header logo component would not accept an SVG so I created a React SVG component instead.
  • The elastic logo SVG code was still in the Template component so I replaced that with the OpenSearch mark.
  • There is still a reference to the heatmap icon in src/plugins/home/public/application/components/tutorial/introduction.test.js but it's unclear how that is used outside of the test or whether it needs to be changed.
  • The welcome logo needed to use the centered mark because of the spacing, but that required changing the padding.
  • I don't yet know how to test the icons used for each of the plugins: dev_tools, opensearch_logs, opensearch_metrics, or the management plugin.

Brand Guidelines: https://opensearch.org/brand.html

Loading

Screen Shot 2021-05-25 at 3 23 09 PM

Welcome

Screen Shot 2021-05-25 at 3 13 49 PM

Header

Screen Shot 2021-05-24 at 2 43 54 PM

Issues Resolved

#370

Check List

  • New functionality includes testing.
    • Unit tests pass
    • Integration tests pass
    • Functional tests pass (WIP)
  • New functionality has been documented.
    • New functionality has javadoc added (N/A)
  • Commits are signed per the DCO using --signoff

@tmarkley tmarkley self-assigned this May 25, 2021
@tmarkley tmarkley added this to the 1.x release milestone May 25, 2021
@tmarkley tmarkley linked an issue May 25, 2021 that may be closed by this pull request
@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed a9661a6

Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

Functional tests are passing. I think overall looks good. It mimics how svgs are import EUI but can probably have some conditionality based on if the user is using darkmode or not

import React from 'react';

export function OpenSearchDashboardsLogoDarkMode() {
return (
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 for draft purposes it's probably like this but we can probably be consistent with the other svgs were an svg file was created and accessed in the react component (without the helper functions):

addBasePath(
 `/plugins/${PLUGIN_ID}/assets/opensearch_dashboards_${appId}_${
   IS_DARK_THEME ? 'dark' : 'light'
   }.svg`
 )}

then we just make this opensearch_dashboards_logo.tsx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you saying that this code would go in header_logo.tsx? Where do the PLUGIN_ID, appId, and IS_DARK_THEME props get passed in to the component from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And how do I test enabling/disabling dark mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to get these changes out for 1.0, I've added a follow-up task to the meta issue to address this feature for darkmode/default logos based on the theme: #372

@tmarkley tmarkley marked this pull request as ready for review May 26, 2021 20:09
Copy link
Member

@ananzh ananzh left a comment

Choose a reason for hiding this comment

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

LGTM

@kavilla kavilla self-requested a review May 26, 2021 21:45
Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

Followup and improvements can be made but good first pass for replacing the heatmap.

@kavilla kavilla merged commit c96cd2d into opensearch-project:main May 26, 2021
@tmarkley tmarkley deleted the 370 branch May 26, 2021 21:47
kavilla pushed a commit that referenced this pull request May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update OpenSearch logos site-wide with new branding
4 participants