-
Notifications
You must be signed in to change notification settings - Fork 72
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
[OUI Docs] Doc site header clean up #118
[OUI Docs] Doc site header clean up #118
Conversation
Signed-off-by: Bandini Bhopi <[email protected]>
Signed-off-by: Bandini Bhopi <[email protected]>
Signed-off-by: Bandini Bhopi <[email protected]>
Signed-off-by: Bandini Bhopi <[email protected]>
Signed-off-by: Bandini Bhopi <[email protected]>
Signed-off-by: Bandini Bhopi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
items: [ | ||
<OuiHeaderLogo iconType="logoElastic">Elastic</OuiHeaderLogo>, | ||
], | ||
items: [<OuiHeaderLogo iconType="logoOUI">Elastic</OuiHeaderLogo>], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should references to Elastic in these files be changed to OpenSearch, or is that out of the scope of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be OpenSearch UI, and this makes me think we actually need an OUI logo. The team has explored some options, we should put them forward
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BSFishy @kgcreative elastic reference removal is in my PR, and the OUI logo is in an upcoming PR. The to-do list on the original issue is accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, there may be merge conflicts between our PRs as I believe @bandinib-amzn originally included some of the elastic removal - I am not sure if those commits have been rolled back or not though. Will connect with @bandinib-amzn when I am not out of pocket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KrooshalUX which elastic removal are we talking about? This PR includes only proposed changes from #111. Did I mistakenly remove other elastic reference? I'm not sure which commit needs to be rolled back.
@@ -237,7 +237,7 @@ const typeToPathMap = { | |||
logoCouchbase: 'logo_couchbase', | |||
logoDocker: 'logo_docker', | |||
logoDropwizard: 'logo_dropwizard', | |||
logoElastic: 'logo_elastic', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep this for the sake of backwards compatibility? Do we know if anyone else is using this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this change is in scope of this PR - there are several edits to the logos file that I am tracking as part of the meta issue for compliance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per last task (Change logoElastic to logoOUI) from #111, I changed all logoElastic
references to logoOUI
. Does it mean that in this PR we should just update logoElastic
to logoOUI
and keep name of logo which is logo_elastic
as it is in this PR?
Close in favor of #164 |
Description
Issues Resolved
#111
Screenshots
Header Changes
Before:
After:
Changed logoElastic to logoOUI
Before:
After:
The blank image is because of a reference to a broken image. @KrooshalUX will commit logo change which will resolve this. Also one UT is failing because of this reference to a broken image. This should also resolve.
Check List
yarn lint
yarn test-unit
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.