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

Remove styling causing issue for banner in advanced settings #488

Conversation

mengweieric
Copy link
Collaborator

@mengweieric mengweieric commented May 26, 2023

Description

Issue:
in advanced setting page of Core Dashboard, if you made a change, a banner should show up on the bottom of the page

Screenshot 2023-05-25 at 5 44 43 PM

but currently in main, it is missing.

Root cause:
Changes in main of dashboard core moved that banner code from a sibling div to under the main body with an id of '#opensearch-dashboards-body'. Observability also refer to this id for its own styling previously affecting that banner.

Versions like 2.7 which do not see such bug have DOM structure like

Screenshot 2023-05-25 at 4 31 56 PM

where banner section with 'label Page level controls' locates out side of 'opensearch-dashboards-body' whereas in main:

Screenshot 2023-05-25 at 4 32 23 PM

It got moved to under body div.

Fix

Removed styling which didn't have actual effect currently for observability but affecting that banner. Also moving forward, observability will add wrapper / its own class name to avoid possible global styling.

Issues Resolved

#484

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • 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.

@codecov
Copy link

codecov bot commented May 26, 2023

Codecov Report

Merging #488 (d459ed5) into main (6f653e2) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #488   +/-   ##
=======================================
  Coverage   42.81%   42.81%           
=======================================
  Files         299      299           
  Lines       17784    17784           
  Branches     4355     4355           
=======================================
  Hits         7615     7615           
  Misses      10128    10128           
  Partials       41       41           
Flag Coverage Δ
dashboards-observability 42.81% <ø> (ø)

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

Copy link
Collaborator

@derek-ho derek-ho left a comment

Choose a reason for hiding this comment

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

Looks good thanks

@mengweieric mengweieric merged commit 952ed38 into opensearch-project:main May 30, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 30, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 30, 2023
ps48 pushed a commit that referenced this pull request May 30, 2023
Signed-off-by: Eric Wei <[email protected]>
(cherry picked from commit 952ed38)

Co-authored-by: Eric Wei <[email protected]>
ps48 pushed a commit that referenced this pull request May 30, 2023
Signed-off-by: Eric Wei <[email protected]>
(cherry picked from commit 952ed38)

Co-authored-by: Eric Wei <[email protected]>
pjfitzgibbons pushed a commit to TackAdam/dashboards-observability that referenced this pull request Jun 5, 2023
@mengweieric mengweieric deleted the issue/missing-banner-in-adv-setting branch February 14, 2024 18:29
amsiglan pushed a commit to amsiglan/dashboards-observability that referenced this pull request Jun 7, 2024
…roject#488) (opensearch-project#501)

Signed-off-by: Eric Wei <[email protected]>
(cherry picked from commit 952ed38)

Co-authored-by: Eric Wei <[email protected]>
(cherry picked from commit 0ac794d)
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.

3 participants