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

[SLD] 237 animate feeder arrows #500

Merged
merged 10 commits into from
Mar 27, 2023
Merged

[SLD] 237 animate feeder arrows #500

merged 10 commits into from
Mar 27, 2023

Conversation

tadam50
Copy link
Contributor

@tadam50 tadam50 commented Feb 13, 2023

Please check if the PR fulfills these requirements (please use '[x]' to check the checkboxes, or submit the PR and then click the checkboxes)

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Does this PR already have an issue describing the problem ? If so, link to this issue using '#XXX' and skip the rest
#237

@tadam50 tadam50 linked an issue Feb 13, 2023 that may be closed by this pull request
@tadam50 tadam50 self-assigned this Feb 13, 2023
@tadam50 tadam50 changed the title 237 animate feeder arrows [SLD] 237 animate feeder arrows Feb 13, 2023
@tadam50 tadam50 requested review from So-Fras and flo-dup February 13, 2023 16:34
@tadam50 tadam50 marked this pull request as ready for review February 13, 2023 16:34
@tadam50
Copy link
Contributor Author

tadam50 commented Feb 13, 2023

Breaking changes is raised here by adding new parameter into
public List<String> getFeederInfoStyles(FeederInfo info, boolean animated)
This new parameter is added to use LayoutParameter::isEnableFeederInfosAnimation()

@tadam50 tadam50 force-pushed the 237-animate-feeder-arrows branch from c6ec577 to 5c2733c Compare February 13, 2023 17:16
Copy link
Member

@So-Fras So-Fras left a comment

Choose a reason for hiding this comment

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

Thank you for your work so far. We must clear up some specifications for you to be able to go on with the development.
Moreover, we did not realize at first that this feature could bring a breaking change.

@@ -46,6 +46,14 @@ public final class DiagramStyles {
public static final String ANGLE = STYLE_PREFIX + "angle";
public static final String FEEDER_INFO = STYLE_PREFIX + "feeder-info";

public static final String ARROW_ANIMATION = STYLE_PREFIX + "arrow-animation";

public static final String ARROW_ANIMATION_SPEED_1 = STYLE_PREFIX + "arrow-animation-speed-1";
Copy link
Member

Choose a reason for hiding this comment

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

We have to decide how to discretize the power values and maybe include "no value" or "zero power" cases.

Copy link
Member

@So-Fras So-Fras left a comment

Choose a reason for hiding this comment

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

I made very few comments. Thank you for your nice work!

Copy link
Member

@So-Fras So-Fras left a comment

Choose a reason for hiding this comment

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

Everything is OK for me!

.sld-arrow-animation-low-speed { --sld-arrow-animation-parameters: 3s infinite linear }
.sld-arrow-animation-average-speed { --sld-arrow-animation-parameters: 2s infinite linear }
.sld-arrow-animation-high-speed { --sld-arrow-animation-parameters: 1s infinite linear }
.sld-arrow-animation-no-speed { --sld-arrow-offset-path: path('') }
Copy link
Contributor

Choose a reason for hiding this comment

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

You could do it simpler by using a not operator instead of introducing one more variable --sld-arrow-offset-path, for instance:

.sld-cell-direction-top .sld-arrow-in :.sld-arrow-animation-no-speed {
  offset-rotate: 0deg;
  offset-path: path('M 0,-10 0,10');
  animation: move var(--sld-arrow-animation-parameters, 0s);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This file is not working on my firefox 96.0, the bottom black arrow is pointing to the top instead of to the bottom. It seems that the rotate is overriden. But in the specifications it is written that:

Note: The rotation described here does not override or replace any rotation defined by the transform property.

So, if it still occurs on a more recent version of firefox, we should report it to mozilla bug report.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Firefox ESR 102.9.0 (March 14, 2023) same issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for testing!

tadam50 added 8 commits March 23, 2023 14:00
Signed-off-by: Thomas ADAM <tadam@silicom.fr>
…nts library

Signed-off-by: Thomas ADAM <tadam@silicom.fr>
Signed-off-by: Thomas ADAM <tadam@silicom.fr>
Signed-off-by: Thomas ADAM <tadam@silicom.fr>
Signed-off-by: Thomas ADAM <tadam@silicom.fr>
Signed-off-by: Thomas ADAM <tadam@silicom.fr>
Signed-off-by: Thomas ADAM <tadam@silicom.fr>
…tion

Signed-off-by: Thomas ADAM <tadam@silicom.fr>
@tadam50 tadam50 force-pushed the 237-animate-feeder-arrows branch 2 times, most recently from 59bc015 to 67f8f01 Compare March 23, 2023 15:44
Signed-off-by: Thomas ADAM <tadam@silicom.fr>
@tadam50 tadam50 force-pushed the 237-animate-feeder-arrows branch from 67f8f01 to 2f3b9ca Compare March 23, 2023 15:50
Copy link
Contributor

@flo-dup flo-dup left a comment

Choose a reason for hiding this comment

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

Just 2 typos and we're good!

@@ -0,0 +1,75 @@
/**
* Copyright (c) 2019, RTE (http://www.rte-france.com)
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace with 2023. Besides we've been asked to add * SPDX-License-Identifier: MPL-2.0 at the end to ease up license scanning

network.getLoad("l2").getTerminal().setP(501.0);
network.getLoad("l2").getTerminal().setQ(0.0);

// Enable animation
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove this comment

Signed-off-by: Thomas ADAM <tadam@silicom.fr>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@flo-dup flo-dup merged commit ba104bc into main Mar 27, 2023
@flo-dup flo-dup deleted the 237-animate-feeder-arrows branch March 27, 2023 18:19
BenoitJeanson pushed a commit that referenced this pull request May 2, 2023
* Use new AnimatedFeederInfoStyleProvider for animation
* Add 2 thresholds in AnimatedFeederInfoStyleProvider constructor
* Update svg test reference files
* HvdcTest : fix svg generation at any time

Signed-off-by: Thomas ADAM <tadam@silicom.fr>
Signed-off-by: BenoitJeanson <benoit.jeanson@rte-france.com>
BenoitJeanson pushed a commit that referenced this pull request May 4, 2023
* Use new AnimatedFeederInfoStyleProvider for animation
* Add 2 thresholds in AnimatedFeederInfoStyleProvider constructor
* Update svg test reference files
* HvdcTest : fix svg generation at any time

Signed-off-by: Thomas ADAM <tadam@silicom.fr>
BenoitJeanson pushed a commit that referenced this pull request May 4, 2023
* Use new AnimatedFeederInfoStyleProvider for animation
* Add 2 thresholds in AnimatedFeederInfoStyleProvider constructor
* Update svg test reference files
* HvdcTest : fix svg generation at any time

Signed-off-by: Thomas ADAM <tadam@silicom.fr>
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.

Animate feeder arrows
3 participants