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

Feat (Advanced Settings): Make new "Appearance" category #4845

Merged

Conversation

joshuarrrr
Copy link
Member

@joshuarrrr joshuarrrr commented Aug 28, 2023

Description

And remove "Accessibility".
Give category panels IDs so they can be deep linked

Copy and other settings updates will follow in separate PRs.

Issues Resolved

Partial implementation of #4330
Unblocks #4676

Screenshot

Note that advanced settings are automatically ordered alphabetically by the setting name.

Screen.Recording.2023-08-28.at.3.20.10.PM.mov

Testing the changes

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

And remove "Accessibility".
Give category panels IDs so they can be deep linked

Signed-off-by: Josh Romero <[email protected]>
@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Merging #4845 (9957efa) into main (ce2d79e) will decrease coverage by 0.01%.
Report is 4 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #4845      +/-   ##
==========================================
- Coverage   66.39%   66.39%   -0.01%     
==========================================
  Files        3396     3397       +1     
  Lines       64801    64804       +3     
  Branches    10359    10360       +1     
==========================================
+ Hits        43025    43026       +1     
- Misses      19217    19218       +1     
- Partials     2559     2560       +1     
Flag Coverage Δ
Linux_1 34.85% <ø> (ø)
Linux_2 55.15% <ø> (ø)
Linux_3 44.23% <ø> (-0.01%) ⬇️
Linux_4 34.92% <ø> (ø)
Windows_1 34.87% <ø> (ø)
Windows_2 55.12% <ø> (ø)
Windows_3 44.24% <ø> (+<0.01%) ⬆️
Windows_4 34.92% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
.../core/server/ui_settings/settings/accessibility.ts 100.00% <ø> (ø)
src/core/server/ui_settings/settings/navigation.ts 100.00% <ø> (ø)
src/core/server/ui_settings/settings/theme.ts 100.00% <ø> (ø)
...ngs/public/management_app/components/form/form.tsx 69.69% <ø> (ø)
...ngs/public/management_app/lib/get_category_name.ts 66.66% <ø> (ø)

... and 3 files with indirect coverage changes

@joshuarrrr
Copy link
Member Author

@KrooshalUX This is ready for your review. Note that it's not everything from #4330, but it does have everything moved to the new section, which is deep linkable. Given this minimal implementation, should I go ahead and also update the copy to match the mock (except for the browser settings, which won't be supported in 2.10).

@mnkugler The issue mentions that we want the theme description text to link to a forum post. Can you provide that forum post URL?

@KrooshalUX
Copy link

👍 this is great incremental progress towards the north star of appearance within advanced settings (#4330 )!

@kavilla
Copy link
Member

kavilla commented Aug 29, 2023

Not sure if this a current issue but will we move forward with the other plugins to add to this category?

Signed-off-by: Josh Romero <[email protected]>
@ashwin-pc
Copy link
Member

@joshuarrrr Some of the unit tests are failing here, can you look at them?

add link to forum

Signed-off-by: Josh Romero <[email protected]>
Signed-off-by: Josh Romero <[email protected]>
@joshuarrrr
Copy link
Member Author

Not sure if this a current issue but will we move forward with the other plugins to add to this category?

Are you asking whether plugins can add additional settings to this Appearance category? If so, the answer is yes, which is consistent with how the other categories work.

AMoo-Miki
AMoo-Miki previously approved these changes Aug 30, 2023
ashwin-pc
ashwin-pc previously approved these changes Aug 31, 2023
@@ -53,9 +54,11 @@ export const getThemeSettings = (): Record<string, UiSettingsParams> => {
type: 'select',
options: ['v7', 'Next (preview)'],
description: i18n.translate('core.ui_settings.params.themeVersionText', {
defaultMessage: `Switch between the theme used for the current and next version of OpenSearch Dashboards, A page refresh is required for the setting to be applied.`,
defaultMessage: `<p>Switch between the theme used for the current and next version of OpenSearch Dashboards. A page refresh is required for the setting to be applied.</p><p><a href="{href}">{linkText}</a></p>`,
values: { href: 'https://forum.opensearch.org/', linkText: 'Theme feedback' },
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: update href with specific forum post.

I thought I could use the linkService here, but it's a case where we can't really; these settings are defined in /core/server and the linkService is in /core/public.

Copy link
Member Author

Choose a reason for hiding this comment

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

href updated

src/core/server/ui_settings/settings/theme.ts Outdated Show resolved Hide resolved
Signed-off-by: Josh Romero <[email protected]>
@joshuarrrr joshuarrrr dismissed stale reviews from ashwin-pc and AMoo-Miki via 7a835a9 August 31, 2023 17:27
ashwin-pc
ashwin-pc previously approved these changes Aug 31, 2023
@joshuarrrr joshuarrrr merged commit 3e1ef80 into opensearch-project:main Sep 1, 2023
@joshuarrrr joshuarrrr deleted the feat/advanced-settings-theming branch September 1, 2023 21:30
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 1, 2023
* Feat (Advanced Settings): Make new "Appearance" category

And remove "Accessibility".
Give category panels IDs so they can be deep linked

Signed-off-by: Josh Romero <[email protected]>

* updated changelog

Signed-off-by: Josh Romero <[email protected]>

* Update theme description

add link to forum

Signed-off-by: Josh Romero <[email protected]>

* Update snapshots

Signed-off-by: Josh Romero <[email protected]>

* update forum link

Signed-off-by: Josh Romero <[email protected]>

* Update src/core/server/ui_settings/settings/theme.ts

Co-authored-by: Miki <[email protected]>
Signed-off-by: Josh Romero <[email protected]>

---------

Signed-off-by: Josh Romero <[email protected]>
Co-authored-by: Miki <[email protected]>
Co-authored-by: Miki <[email protected]>
(cherry picked from commit 3e1ef80)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
joshuarrrr pushed a commit that referenced this pull request Sep 1, 2023
* Feat (Advanced Settings): Make new "Appearance" category

And remove "Accessibility".
Give category panels IDs so they can be deep linked

Signed-off-by: Josh Romero <[email protected]>

* updated changelog

Signed-off-by: Josh Romero <[email protected]>

* Update theme description

add link to forum

Signed-off-by: Josh Romero <[email protected]>

* Update snapshots

Signed-off-by: Josh Romero <[email protected]>

* update forum link

Signed-off-by: Josh Romero <[email protected]>

* Update src/core/server/ui_settings/settings/theme.ts

Co-authored-by: Miki <[email protected]>
Signed-off-by: Josh Romero <[email protected]>

---------

Signed-off-by: Josh Romero <[email protected]>
Co-authored-by: Miki <[email protected]>
Co-authored-by: Miki <[email protected]>
(cherry picked from commit 3e1ef80)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.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.

6 participants