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

[BUG][Console] Fix updating of play and wrench button #3920

Closed

Conversation

kavilla
Copy link
Member

@kavilla kavilla commented Apr 21, 2023

Description

Fix UI bug that did not update the console with the correct values.

offsetTop was always 0 and the value needed the pixel.

Issues Resolved

closes: #3916

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

Fix UI bug that did not update the console with the correct
values.

offsetTop was always 0 and the value needed the pixel.

Issue:
opensearch-project#3916

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
@kavilla
Copy link
Member Author

kavilla commented Apr 21, 2023

links failing from things not added.

@codecov-commenter
Copy link

codecov-commenter commented Apr 21, 2023

Codecov Report

Merging #3920 (89ca895) into main (712a42f) will increase coverage by 0.05%.
The diff coverage is 66.66%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #3920      +/-   ##
==========================================
+ Coverage   66.38%   66.43%   +0.05%     
==========================================
  Files        3227     3227              
  Lines       62051    62051              
  Branches     9593     9593              
==========================================
+ Hits        41191    41223      +32     
+ Misses      18553    18526      -27     
+ Partials     2307     2302       -5     
Flag Coverage Δ
Linux 66.37% <66.66%> (-0.01%) ⬇️
Windows 66.38% <66.66%> (?)

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

Impacted Files Coverage Δ
...on/models/legacy_core_editor/legacy_core_editor.ts 56.94% <66.66%> (ø)

... and 9 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@joshuarrrr
Copy link
Member

@kavilla Was this a regression from one of the recent changes to the console or a latent bug? If it's a regression, it would be nice to link the PR here for context. Also, if you have a little animation of the fixed behavior, that'd be 🥇 .

@joshuarrrr
Copy link
Member

ah, yeah, this is the PR: #3733

@kavilla
Copy link
Member Author

kavilla commented Apr 22, 2023

@kavilla Was this a regression from one of the recent changes to the console or a latent bug? If it's a regression, it would be nice to link the PR here for context.

Yeah this one: #3733

Basically we previously got a number and jquery would automagically add the px if it was a number.

joshuarrrr
joshuarrrr previously approved these changes Apr 22, 2023
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.

This looks good, but I don't think we need a release note - it's just a fix/continuation of the jquery removal PR already included.

release-notes/opensearch-dashboards.release-notes-2.7.0.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
kavilla and others added 2 commits April 21, 2023 17:57
Co-authored-by: Josh Romero <rmerqg@amazon.com>
Co-authored-by: Josh Romero <rmerqg@amazon.com>
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

This needs tests.

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.

This fix is not sufficient. Instead, we should revert b7af56f, because autocompletion and documentation links are also broken with that commit.

@kavilla
Copy link
Member Author

kavilla commented Apr 24, 2023

Closing for #3929

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.

[BUG] Play/Wrench Button Regression in Dev Tools
5 participants