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

JBPM-8903 : Stunner - Properties panel is not opened after clicking on "Minimize panel" button #3127

Merged
merged 1 commit into from Feb 6, 2020

Conversation

inodeman
Copy link
Contributor

Hi @romartin Can you review this PR?

@inodeman
Copy link
Contributor Author

Jenkins execute full downstream build

2 similar comments
@inodeman
Copy link
Contributor Author

Jenkins execute full downstream build

@inodeman
Copy link
Contributor Author

Jenkins execute full downstream build

@romartin
Copy link
Contributor

@romartin @LuboTerifaj This is scoped for 7.7 - please review

@inodeman
Copy link
Contributor Author

Jenkins execute full downstream build

Copy link
Contributor

@romartin romartin left a comment

Choose a reason for hiding this comment

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

LGTM @inodeman , thanks!! 👍

@romartin
Copy link
Contributor

@inodeman can you please generate the kie-wb-distributions WAR (Business Central) locally and upload it somewhere, so we can also test the UI? (meanwhile FDB are failing).
Thanks!

@inodeman
Copy link
Contributor Author

Jenkins execute full downstream build

@inodeman
Copy link
Contributor Author

@inodeman
Copy link
Contributor Author

Hi @romartin war was uploaded, can you review it?

@romartin
Copy link
Contributor

thanks @inodeman . Anyway now FDB is green, so @LuboTerifaj can also start the review. Pllease guys, tthat's a priority for product. Thanks!

Copy link
Contributor

@LuboTerifaj LuboTerifaj left a comment

Choose a reason for hiding this comment

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

Hello @inodeman ,

I have found a few issues:

  • Properties panel is open instead of Explore Diagram panel.
    Steps to reproduce:

    1. Ensure that Properties panel is open.
    2. Maximize panel.
    3. Open Explore Diagram panel.
    4. Minimize panel.
  • Explore diagram panel is not open. There is also an animation that is not needed.
    Steps to reproduce:

    1. Ensure that Explore diagram panel is open.
    2. Maximize panel.
    3. Minimize panel.
  • There is an animation that is not needed.
    Steps to reproduce:

    1. Ensure that Properties panel is closed.
    2. Maximize panel.
    3. Open Properties or Explore Diagram panel.
    4. Minimize panel.

One thought came to my mind. It is not clear, what behaviour should be implemented when switching between different assets. However this cannot be resolved until https://issues.redhat.com/browse/JBPM-8915 is done. I consider it as out of scope of the jira.

Can you please have a look at my findings?

Thank you!

@LuboTerifaj
Copy link
Contributor

Hello @inodeman , I have found one more issue.

Switch to full screen mode, close Properties panel and then repeat 3 times:

  1. Maximize panel.
  2. Hide alerts.
  3. View alerts.
  4. Minimize panel.
    Animation that is not needed is being shown.

After that, execute just first two steps.
The whole designer is shifted. See the picture:
shifted-designer

@inodeman
Copy link
Contributor Author

Hi @LuboTerifaj some of those issues I think happen with or without this PR, I think we should create Jira for it. Now for the Explore Panel I did not see it mentioned on the Ticket, also the behaviour about when the window is maximized and the user clicks properties panel and then on minimize does seem logical but was not mentioned previously. I think I can do the Explore Panel plus the behaviour of user clicking on Properties when Maximized in a few hours, but in case it takes more, I think could be a good idea to merge the PR and add the issues on Jiras, what do you think Lubo, @romartin ?

@inodeman
Copy link
Contributor Author

inodeman commented Jan 30, 2020

@LuboTerifaj I saw the issue you comment about the panel being shifted, it is due to Uberfire and its docks it seems, although it only shows for about half a second and then it goes back to normal in my case. Now, I have seen from time to time that issue you comment where it just stays shifted. On your case is it reproducible 100% of the time? I mean that it stays shifted?

Also, the animation that is not needed seems it also comes from Uberfire, where it tries to minimize the main panel as if it was going to open properties panel but then it closes. I will research a bit more

@inodeman
Copy link
Contributor Author

Jenkins execute full downstream build

@inodeman
Copy link
Contributor Author

Hi @LuboTerifaj I added the functionality you mentioned on the Explore Panel, so it should work now, about all other issues can we create other Jiras for them? let me know

@mbiarnes
Copy link
Contributor

Jenkins execute full downstream build

@LuboTerifaj
Copy link
Contributor

Hello @inodeman , can you apply kie code style formatting on your code please? Thanks!

@romartin
Copy link
Contributor

romartin commented Feb 3, 2020

@inodeman plz reformat code andl let us know agian. Thx!

Copy link
Contributor

@LuboTerifaj LuboTerifaj left a comment

Choose a reason for hiding this comment

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

Hello @inodeman ,

I've reported the issues:
JBPM-9005 Maximize and minimize panel in full screen mode shifts the whole designer
JBPM-9004 Redundant animation is shown after panel is minimized
JBPM-9003 Properties panel is open instead of Explore Diagram panel

Once, the proper code style is applied it will be ready to merge.

Thanks!

@inodeman
Copy link
Contributor Author

inodeman commented Feb 4, 2020

Jenkins execute full downstream build

@inodeman
Copy link
Contributor Author

inodeman commented Feb 4, 2020

Hi @LuboTerifaj @romartin I re-checked the formatting and I have it set as KIE by default, changed a couple of conflicting issues, I think should be fine now, let me know

@romartin
Copy link
Contributor

romartin commented Feb 4, 2020

Jenkins retest this

@romartin
Copy link
Contributor

romartin commented Feb 4, 2020

ok @LuboTerifaj please missing your final review! thx.

@sonarcloud
Copy link

sonarcloud bot commented Feb 5, 2020

Kudos, SonarCloud Quality Gate passed!

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

75.0% 75.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@LuboTerifaj LuboTerifaj left a comment

Choose a reason for hiding this comment

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

Hello @inodeman ,

the kie code style has not been applied in the all changed files.
See inline comments for more information.

However, it doesn't affect quality of implemented fix, hence I am for merging the pull-request.

Thanks!

@@ -172,7 +172,7 @@ public void checkDestructionReleasesResources() {

@Test
public void checkOnScreenMaximisedDiagramEditor() {
final ScreenMaximizedEvent event = new ScreenMaximizedEvent(true);
final ScreenMaximizedEvent event = new ScreenMaximizedEvent(true );
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
final ScreenMaximizedEvent event = new ScreenMaximizedEvent(true );
final ScreenMaximizedEvent event = new ScreenMaximizedEvent(true);

super.doOpen();
.filter(dock -> dock.getPlaceRequest().getIdentifier().compareTo(id) == 0)
.forEach(action);

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change


private final boolean isExplorerScreen;

public ScreenPreMaximizedStateEvent(final boolean isExplorerScreen ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public ScreenPreMaximizedStateEvent(final boolean isExplorerScreen ) {
public ScreenPreMaximizedStateEvent(final boolean isExplorerScreen) {

@romartin romartin merged commit fed2372 into kiegroup:master Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants