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

Fix (styles): Make ace editor theme consistent for Search Relevance plugin #300

Merged
merged 2 commits into from
Oct 6, 2023

Conversation

nung22
Copy link
Contributor

@nung22 nung22 commented Sep 30, 2023

Description

Fixes inconsistent editor theming in dark mode for Search Relevance plugin by using "textmate" theme rather than "sql_console". Builds off solution discussed in opensearch-project/OpenSearch-Dashboards#4609.

Issues Resolved

Closes #307

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.

Screenshots

Before

Editor is still in light mode while site is in dark mode:

Editor is inconsistent with dark theme.

After

Editor and site are now both in dark mode:

Editor is consistent with dark theme.

nung22 and others added 2 commits September 30, 2023 20:31
Signed-off-by: Nicholas Ung <[email protected]>
Signed-off-by: Nicholas Ung <[email protected]>
Signed-off-by: Nicholas Ung <[email protected]>
@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Merging #300 (1b72fc3) into main (26f1ea0) will decrease coverage by 2.95%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #300      +/-   ##
==========================================
- Coverage   86.27%   83.33%   -2.95%     
==========================================
  Files          15       15              
  Lines         204      204              
  Branches       40       40              
==========================================
- Hits          176      170       -6     
- Misses         26       32       +6     
  Partials        2        2              
Flag Coverage Δ
dashboards-search-relevance 83.33% <ø> (-2.95%) ⬇️

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

Files Coverage Δ
...search_components/search_configs/search_config.tsx 50.00% <ø> (ø)

... and 1 file with indirect coverage changes

Copy link
Member

@sejli sejli left a comment

Choose a reason for hiding this comment

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

Nice work @nung22! Out of curiosity, did you update the test snapshots manually or by running yarn test -u?

Changes look good to me. The integration tests are failing for all PRs; we're investigating the issue currently. I'll approve this now and merge this in once we have a solution to the integration test failures.

@nung22
Copy link
Contributor Author

nung22 commented Oct 2, 2023

Nice work @nung22! Out of curiosity, did you update the test snapshots manually or by running yarn test -u?

Changes look good to me. The integration tests are failing for all PRs; we're investigating the issue currently. I'll approve this now and merge this in once we have a solution to the integration test failures.

Sounds good! I updated the snapshots manually after I realized some of the tests were failing with my initial changes. Wasn't aware that you could update them with yarn test -u. Should I do that now?

@macohen macohen added the v2.11.0 label Oct 4, 2023
@macohen
Copy link
Collaborator

macohen commented Oct 5, 2023

@nung22, @sejli the 2.11 release window opens today which means this PR needs to be merged today as well. Is that still possible?

@nung22
Copy link
Contributor Author

nung22 commented Oct 5, 2023

@nung22, @sejli the 2.11 release window opens today which means this PR needs to be merged today as well. Is that still possible?

@macohen Based on my prior conversations with @sejli, we should be good to merge this PR.

@sejli
Copy link
Member

sejli commented Oct 6, 2023

Needed to change the integration tests a little bit since Cypress was having trouble connecting to browser, but tests have been run and passed. Will merge now.

 (Run Starting)

  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ Cypress:        9.5.4                                                                          │
  │ Browser:        Chrome 117 (headless)                                                          │
  │ Node Version:   v14.20.0 (/Users/lnse/.nvm/versions/node/v14.20.0/bin/node)                    │
  │ Specs:          1 found (plugins/search-relevance-dashboards/1_query_compare.spec.js)          │
  │ Searched:       cypress/integration/plugins/search-relevance-dashboards/1_query_compare.spec.j │
  │                 s                                                                              │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘


────────────────────────────────────────────────────────────────────────────────────────────────────
                                                                                                    
  Running:  plugins/search-relevance-dashboards/1_query_compare.spec.js                     (1 of 1)
Browserslist: caniuse-lite is outdated. Please run:
npx browserslist@latest --update-db

Why you should do it regularly:
https://github.com/browserslist/browserslist#browsers-data-updating
Couldn't determine Mocha version


  Compare queries
    ✓ Should get comparison results (99713ms)


  1 passing (2m)


  (Results)

  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ Tests:        1                                                                                │
  │ Passing:      1                                                                                │
  │ Failing:      0                                                                                │
  │ Pending:      0                                                                                │
  │ Skipped:      0                                                                                │
  │ Screenshots:  0                                                                                │
  │ Video:        true                                                                             │
  │ Duration:     2 minutes, 20 seconds                                                            │
  │ Spec Ran:     plugins/search-relevance-dashboards/1_query_compare.spec.js                      │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘


  (Video)

  -  Started processing:  Compressing to 32 CRF                                                     
  -  Finished processing: /Users/lnse/opensearch/opensearch-dashboards-functional-tes    (6 seconds)
                          t/cypress/videos/plugins/search-relevance-dashboards/1_quer               
                          y_compare.spec.js.mp4                                                     


====================================================================================================

  (Run Finished)


       Spec                                              Tests  Passing  Failing  Pending  Skipped  
  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ ✔  plugins/search-relevance-dashboards      02:20        1        1        -        -        - │
  │    /1_query_compare.spec.js                                                                    │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘
    ✔  All specs passed!                        02:20        1        1        -        -        -  

✨  Done in 167.98s.

@sejli sejli merged commit 0b5d9f3 into opensearch-project:main Oct 6, 2023
11 of 13 checks passed
github-actions bot added a commit that referenced this pull request Oct 6, 2023
…lugin (#300)

* Update code editor theme to textmate

Signed-off-by: Nicholas Ung <[email protected]>
Signed-off-by: Nicholas Ung <[email protected]>

* Update theme in testing file

Signed-off-by: Nicholas Ung <[email protected]>

---------

Signed-off-by: Nicholas Ung <[email protected]>
Signed-off-by: Nicholas Ung <[email protected]>
(cherry picked from commit 0b5d9f3)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@nung22 nung22 deleted the fix/issue-257 branch October 6, 2023 01:36
sejli pushed a commit that referenced this pull request Oct 6, 2023
…lugin (#300) (#314)

* Update code editor theme to textmate




* Update theme in testing file



---------



(cherry picked from commit 0b5d9f3)

Signed-off-by: Nicholas Ung <[email protected]>
Signed-off-by: Nicholas Ung <[email protected]>
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>
github-actions bot added a commit that referenced this pull request Oct 11, 2023
…lugin (#300)

* Update code editor theme to textmate

Signed-off-by: Nicholas Ung <[email protected]>
Signed-off-by: Nicholas Ung <[email protected]>

* Update theme in testing file

Signed-off-by: Nicholas Ung <[email protected]>

---------

Signed-off-by: Nicholas Ung <[email protected]>
Signed-off-by: Nicholas Ung <[email protected]>
(cherry picked from commit 0b5d9f3)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@github-actions
Copy link

The backport to 2.11 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/dashboards-search-relevance/backport-2.11 2.11
# Navigate to the new working tree
pushd ../.worktrees/dashboards-search-relevance/backport-2.11
# Create a new branch
git switch --create backport-300-to-2.11
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0b5d9f3b67961d0473497f062a6874eb0d39ee6b
# Push it to GitHub
git push --set-upstream origin backport-300-to-2.11
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/dashboards-search-relevance/backport-2.11

Then, create a pull request where the base branch is 2.11 and the compare/head branch is backport-300-to-2.11.

harshavamsi pushed a commit to harshavamsi/dashboards-search-relevance that referenced this pull request Oct 11, 2023
…lugin (opensearch-project#300)

* Update code editor theme to textmate

Signed-off-by: Nicholas Ung <[email protected]>
Signed-off-by: Nicholas Ung <[email protected]>

* Update theme in testing file

Signed-off-by: Nicholas Ung <[email protected]>

---------

Signed-off-by: Nicholas Ung <[email protected]>
Signed-off-by: Nicholas Ung <[email protected]>
(cherry picked from commit 0b5d9f3)
noCharger pushed a commit that referenced this pull request Oct 11, 2023
…lugin (#300) (#327)

* Update code editor theme to textmate




* Update theme in testing file



---------



(cherry picked from commit 0b5d9f3)

Signed-off-by: Nicholas Ung <[email protected]>
Signed-off-by: Nicholas Ung <[email protected]>
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

(OUI Next Theme) Code Editor Not Compliant with Dark Mode
4 participants