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

Reposition Loading Indicator #970

Merged
merged 12 commits into from
Jul 19, 2022
Merged

Conversation

cvetankanechevska
Copy link
Contributor

@cvetankanechevska cvetankanechevska commented Jul 13, 2022

Signed-off-by: Cvetanka [email protected]

Description

Fix #779

Development notes

Style fix for positioning LoadingIcon to the center of Flowchart and make it responsive as Sidebar can be open/close.

QA notes

I hardcoded the value so it can be visible for testing. After this PR is approved, I will return it as it was.

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Signed-off-by: Cvetanka <[email protected]>
@comym
Copy link

comym commented Jul 14, 2022

Hey hey, thanks for that!

It's pretty much there but the loader is not really central to the flowchart canvas. It should be a bit to the left.

Screenshot 2022-07-14 at 10 21 25

The area we should consider is the one marked as this lime colour as seen in the images. Basically the area we should consider as width reference is everything from the end of the utility bar all the way to the right edge of the screen.

Screenshot 2022-07-14 at 10 21 49

Many thanks, let me know if you have any questions.

@tynandebold
Copy link
Member

Thanks for this!

It still looks a little off center to me. I think that's because the container element is overlapping part of the slidebar and toolbar:

image

As Gabriel said, it should only cover the flowchart and nothing more. Then it should align perfectly.

@cvetankanechevska
Copy link
Contributor Author

Thanks for this!

It still looks a little off center to me. I think that's because the container element is overlapping part of the slidebar and toolbar:

image

As Gabriel said, it should only cover the flowchart and nothing more. Then it should align perfectly.

Yes, I see. I am using calc() for calculating the width of flowchart and sidebar. But I am missing something, so I will recheck it again.

Copy link
Member

@tynandebold tynandebold left a comment

Choose a reason for hiding this comment

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

Looks perfect now, thank you.

Comment on lines 4 to 6
$total-sidebar-toolbar-width-closed: $sidebar-width-closed +
$global-toolbar-width;
$total-sidebar-toolbar-width-open: $sidebar-width-open + $global-toolbar-width;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think we can get away without total- here.

Suggested change
$total-sidebar-toolbar-width-closed: $sidebar-width-closed +
$global-toolbar-width;
$total-sidebar-toolbar-width-open: $sidebar-width-open + $global-toolbar-width;
$sidebar-toolbar-width-closed: $sidebar-width-closed +
$global-toolbar-width;
$sidebar-toolbar-width-open: $sidebar-width-open + $global-toolbar-width;

Signed-off-by: Cvetanka <[email protected]>
Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

hi @cvetankanechevska -- for me, even after the flowchart is loaded, the logo still shows in the center and doesn't disappear. I can show it to u on call, if u have some time today.

@rashidakanchwala rashidakanchwala self-requested a review July 18, 2022 09:13
Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

LGTM!

Signed-off-by: Cvetanka <[email protected]>
bottom: 20px;
top: 0;
transform: translateX($sidebar-toolbar-width-closed);
transition: transform 0.4s ease, width ease 0.4s;
Copy link
Contributor

Choose a reason for hiding this comment

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

hey @cvetankanechevska I'm just thinking about whether it needs to transition width here? It's normally quite bad for the page performance as the property triggers layout or paint 🤔

I'm not sure whether it's possible to just set a fix width and use translate to move it around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @Huongg, yes we need it to be a smooth reposition when opening/closing the Sidebar

Signed-off-by: Cvetanka <[email protected]>
@cvetankanechevska cvetankanechevska merged commit 30c3894 into main Jul 19, 2022
@cvetankanechevska cvetankanechevska deleted the fix/reposition-loading-indicator branch July 19, 2022 12:06
@tynandebold tynandebold mentioned this pull request Jul 29, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FE] Reposition loading indicator
5 participants