Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

fix: Timestamp/range being re-opened on window resize. #432

Merged

Conversation

peter-affenzeller
Copy link
Contributor

Pull Request


Hi, thank you for contributing to Barista with this pull request (PR).

To ensure a fast process and merging of your PR please make sure it fulfills the
coding standards and contribution guidelines.

  • A feature proposal has been provided, discussed and approved first.
  • There is a meaningful description of the issue in GitHub (Screenshots are
    often helpful).
  • If the PR introduces breaking-changes or deprecations it matches the following
    guidelines.
    • The commit message follows our commit guidelines.
    • Tests for the changes have been added (for bug fixes / features).
    • Docs have been added / updated (for bug fixes / features).

Please choose the type appropriate for the changes below:

Type of PR

Bugfix (non-breaking change which fixes an issue)

Checklist

  • I have read the CONTRIBUTING doc and I follow the PR guidelines
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@tomheller
Copy link
Collaborator

@peter-affenzeller Our cirlceci trial has run out, which is why we cannot run builds on xlarge resources any more. Could you update your branch from the dynatrace-oss/barista:master to get the fix for this problem?

Copy link
Contributor

@lukasholzer lukasholzer left a comment

Choose a reason for hiding this comment

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

Please Provide an issue for what you want to accomplish with this pr (maybe with a short video about the not working behaviour).

Regarding the changes you made in the components/chart/src/selection-area/selection-area.ts it is not clear for me what you want to accomplish with that. What you have written cannot work.

Please provide an explanation of the stream changes from line 803 to 873 in psudo code.
Explain what does not work out currently where you see the problem and what would your approach on fixing this behaviour.

@lukasholzer lukasholzer added pr: needs-changes This PR has been reviewed and needs changes before it can be merged pr: needs-issue labels Jan 21, 2020
@thomaspink
Copy link
Contributor

As @lukasholzer said, please provide either a description in the PR or an issue.

@lara-aigmueller lara-aigmueller removed the pr: needs-changes This PR has been reviewed and needs changes before it can be merged label Feb 11, 2020
@lukasholzer lukasholzer added the pr: needs-rebase This PR needs rebasing label Feb 11, 2020
@lukasholzer
Copy link
Contributor

@peter-affenzeller please rebase with current master and resolve conflicts for further reviewing

@lukasholzer lukasholzer added the pr: needs-changes This PR has been reviewed and needs changes before it can be merged label Feb 11, 2020
@tomheller tomheller linked an issue Feb 12, 2020 that may be closed by this pull request
@peter-affenzeller peter-affenzeller force-pushed the fix/resize-reopening-overlay branch 4 times, most recently from c4fcc34 to 709b5fe Compare February 14, 2020 10:13
@dynatrace-oss dynatrace-oss deleted a comment from vercel bot Feb 17, 2020
Copy link
Contributor

@lukasholzer lukasholzer left a comment

Choose a reason for hiding this comment

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

Small minor changes to reduce the flaky e2e tests that you have written.

And please add one more scenario like described in the connected issue #96.

After you have done those changes please rebase and reword your commit message that it meets our contribution guidelines.

You should provide the scope you are working fix(chart): ... to the message start and add the issue in the bottom description.

For example:

fix:(chart): Fixes an issue where the chart selection was re-opened on window resize.

Provide a more detailed description.
Fixes #{issue-number}

Sorry for the tedious commit/rebase madness but we need proper commit messages to generate our changelog out of it. So that people understand what we have fixed with this commit.

@@ -456,6 +457,11 @@ export class DtChartSelectionArea implements AfterContentInit, OnDestroy {

/** If there is an overlay open it will dispose it and destroy it */
private _closeOverlay(): void {
// remove class showing the arrows of the range handles
if (this._chart._range) {
this._chart._range._reflectRangeReleased(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@peter-affenzeller I tried to finish your PR as requested, but when I removed this part of the code, the test failed. Did you figure out, why this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lukasholzer @tomheller this is intended to remove the class that makes the little arrows of the drag handles fade in so that they fade in every time the range is opened (also with keyboard input. currently, when you focus the chart and convert a timestamp to a range they don't show), in combination with the reflectRangeReleased(true) in line 334. but I just checked again, this also seems to not work properly, probably better to do this in another PR.

components/chart/src/selection-area/selection-area.ts Outdated Show resolved Hide resolved
@tomheller tomheller self-assigned this Feb 18, 2020
@tomheller tomheller force-pushed the fix/resize-reopening-overlay branch 3 times, most recently from 10e8e86 to 8449adc Compare February 18, 2020 14:02
@tomheller tomheller removed pr: needs-changes This PR has been reviewed and needs changes before it can be merged pr: needs-rebase This PR needs rebasing labels Feb 18, 2020
@tomheller
Copy link
Collaborator

@lukasholzer The flaky end2end test should be solved, when #610 is merged and rebased, as this deals with the update of the overlay, when change detection runs.

resizing the window.

When closing a timestamp or range and then resizing the window, the overlay is re-opened.
This PR fixes that issue by splitting up the stream that fires when a timestamp/range changes
and the window resize stream and only updating the overlay on resize if it actually exists.

Fixes dynatrace-oss#472
@sonarcloud
Copy link

sonarcloud bot commented Feb 19, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 2 Security Hotspots to review)
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@lukasholzer lukasholzer left a comment

Choose a reason for hiding this comment

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

Great Work! @tomheller thanks for finishing this 🐳

@lukasholzer lukasholzer added the pr: merge-ready This PR is ready to be merged label Feb 24, 2020
@tomheller tomheller added pr: needs-cherry-pick When a pull request needs manual cherry picking target: minor This PR is targeted for the next minor release target: patch This PR is targeted for the next patch release labels Feb 24, 2020
@tomheller tomheller merged commit 732cb40 into dynatrace-oss:master Feb 24, 2020
@tomheller
Copy link
Collaborator

Pull request commit 732cb40b914854890575fceb8e878a3d32c24f66 has been cherry picked to 5.x and 5.2.x

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr: merge-ready This PR is ready to be merged pr: needs-cherry-pick When a pull request needs manual cherry picking target: minor This PR is targeted for the next minor release target: patch This PR is targeted for the next patch release
Projects
None yet
5 participants