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.

Sign up for GitHub

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

Jump to bottom

[New Theme] Rename to Next #835

Merged
merged 1 commit into from
Jun 20, 2023
Merged

[New Theme] Rename to Next #835

merged 1 commit into from
Jun 20, 2023

Conversation

AMoo-Miki
Copy link
Collaborator

@AMoo-Miki AMoo-Miki commented Jun 20, 2023 •
edited
Loading

Description

This renames the file and references to the old name.

Also, certain comments that talk about the old beta theme's specifications or limitations are reworded or removed. There are 2 instances that more information is needed before removing some logic.

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • All tests pass
    • yarn lint
    • yarn test-unit
  • Commits are signed per the DCO using --signoff

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.

BSFishy
BSFishy reviewed Jun 20, 2023
View reviewed changes
src-docs/src/theme_next_dark.scss
@@ -9,10 +9,7 @@
* GitHub history for details.
*/

// sass-lint:disable no-url-domains, no-url-protocols
@import url('https://fonts.googleapis.com/css2?family=Inter:wght@300;400;500;600;700&family=Roboto+Mono:ital,wght@0,400;0,700;1,400;1,700&display=swap');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be replaced with Source Sans & Source Code fonts?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, good question - @AMoo-Miki I understand we're not packaging the new fonts with OUI, but do we want to do the same thing where we hot link for the purpose of the doc site?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep! i knew i forgot something. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

To note: here, this is to enable the fonts on the docsite, not to package the fonts with OUI. OUI doesn't package any fonts out of the box, that is up to the implementor

src-docs/src/theme_next_light.scss
@@ -9,10 +9,7 @@
* GitHub history for details.
*/

// sass-lint:disable no-url-domains, no-url-protocols
@import url('https://fonts.googleapis.com/css2?family=Inter:wght@300;400;500;600;700&family=Roboto+Mono:ital,wght@0,400;0,700;1,400;1,700&display=swap');
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as previous comment

joshuarrrr
joshuarrrr reviewed Jun 20, 2023
View reviewed changes
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

Just some minor questions, but looks good.

src-docs/src/theme_next_dark.scss
@@ -9,10 +9,7 @@
* GitHub history for details.
*/

// sass-lint:disable no-url-domains, no-url-protocols
@import url('https://fonts.googleapis.com/css2?family=Inter:wght@300;400;500;600;700&family=Roboto+Mono:ital,wght@0,400;0,700;1,400;1,700&display=swap');
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, good question - @AMoo-Miki I understand we're not packaging the new fonts with OUI, but do we want to do the same thing where we hot link for the purpose of the doc site?

src-docs/src/views/panel/panel_shadow.js
Comment on lines -15 to -18
import { ThemeContext } from '../../components';

export default () => {
const themeContext = useContext(ThemeContext);
Copy link
Member

Choose a reason for hiding this comment

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

What is the theme context, and do we still use it elsewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This guy is responsible for switching between light and dark themes. I donno how we switch from default to "Next" in the docs but will figure it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

To add theme to docsite: 1.1.0...pink-elephant#diff-235116db4d88026739c19ef26262c8b53df2d35cdd4f2ff62d8a8d42bf7ed975R36 and 1.1.0...pink-elephant#diff-393e09a1e9d03ae7b852765c1352fe66181b65d531e054ee0c3f7d4c6cc923a5R45-R48

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, theme context is responsible for switching between themes in general on docsite, not just light/dark

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Matt is right; for OUI, light, dark, next-light, and next-dark are individual themes.

src/components/page/_restrict_width.ts Show resolved Hide resolved
manasvinibs
manasvinibs approved these changes Jun 20, 2023
View reviewed changes
src-docs/src/views/panel/panel_example.js
<OuiCode>hasShadow</OuiCode>. The default theme only allows
removing the border if both <OuiCode>hasShadow</OuiCode> and{' '}
and/or shadow. The default theme only allows removing the border
if both <OuiCode>hasShadow</OuiCode> and{' '}
Copy link
Member

Choose a reason for hiding this comment

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

Are we renaming cascadia theme to next or is it a different theme altogether (default theme)?

I see difference in the comment here. Is it intended?

The Cascadia theme doesn&apos;t allow combining the{' '}
              <OuiCode>hasBorder</OuiCode> option with{' '}
              <OuiCode>hasShadow</OuiCode>.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cas* was a temporary reference to a temporary and incomplete theme; that was reset to default in #816. The new colors are just named "Next"

src-docs/src/views/panel/panel_shadow.js

import { OuiPanel, OuiCode, OuiSpacer } from '../../../../src/components';
import { ThemeContext } from '../../components';

export default () => {
Copy link
Member

Choose a reason for hiding this comment

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

Does it mean we have only one theme now (default one)? and do not need theme context anymore to determine the theme?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The previous beta theme had limitations and ThemeContext was used to apply rules when dealing with those limitations. Next is evolved out of default and as such, has none of those limitations so. there was no need to know the ThemeContext in this file.

@AMoo-Miki
[New Theme] Rename to Next
c7f7c70 
Also:
* Show the Next theme in the docsite

Signed-off-by: Miki <miki@amazon.com>
@joshuarrrr joshuarrrr merged commit 0365b42 into opensearch-project:main Jun 20, 2023
5 checks passed
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Jun 20, 2023
Merged
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 20, 2023
@github-actions
[New Theme] Rename to Next (#835)
5a65dc0 
Also:
* Show the Next theme in the docsite

Signed-off-by: Miki <miki@amazon.com>
(cherry picked from commit 0365b42)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
bandinib-amzn
bandinib-amzn approved these changes Jun 20, 2023
View reviewed changes
Copy link
Member

@bandinib-amzn bandinib-amzn left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the change.

BSFishy pushed a commit that referenced this pull request Jun 20, 2023
@opensearch-trigger-bot @github-actions
[New Theme] Rename to Next (#835) (#837)
e6a2842 
Also:
* Show the Next theme in the docsite


(cherry picked from commit 0365b42)

Signed-off-by: Miki <miki@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@joshuarrrr joshuarrrr mentioned this pull request Jun 21, 2023
Merged
6 tasks
@BSFishy BSFishy mentioned this pull request Jun 30, 2023
Open
@AMoo-Miki AMoo-Miki deleted the rename-new-theme branch September 27, 2023 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshuarrrr joshuarrrr joshuarrrr approved these changes

@BSFishy BSFishy BSFishy approved these changes

@bandinib-amzn bandinib-amzn bandinib-amzn approved these changes

@manasvinibs manasvinibs manasvinibs approved these changes

@seanneumann seanneumann Awaiting requested review from seanneumann

@rednaksi91 rednaksi91 Awaiting requested review from rednaksi91

@KrooshalUX KrooshalUX Awaiting requested review from KrooshalUX

@ashwin-pc ashwin-pc Awaiting requested review from ashwin-pc ashwin-pc is a code owner

@andreymyssak andreymyssak Awaiting requested review from andreymyssak

@SergeyMyssak SergeyMyssak Awaiting requested review from SergeyMyssak

Assignees

@AMoo-Miki AMoo-Miki

Labels
backport 1.x seasoned-contributor
Projects
Look & Feel
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AMoo-Miki @joshuarrrr @BSFishy @bandinib-amzn @manasvinibs