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.

Sign up for GitHub

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

Reposition Loading Indicator #970

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
25ec3f1
Fix LoadingIcon position
cvetankanechevska Jul 13, 2022
ce6173f
Set to be visible - only for testing
cvetankanechevska Jul 13, 2022
0a39e0b
Update Release.md
cvetankanechevska Jul 13, 2022
cc66f39
Add global-toolbar width to calc
cvetankanechevska Jul 13, 2022
4e479d1
Merge branch 'main' into fix/reposition-loading-indicator
cvetankanechevska Jul 14, 2022
0ef3a0b
Change width reposition value
cvetankanechevska Jul 14, 2022
529c866
Merge branch 'fix/reposition-loading-indicator' of github.com:kedro-o…
cvetankanechevska Jul 14, 2022
25bb5d1
Fix width reposition value
cvetankanechevska Jul 14, 2022
dca9dc9
Rename variable
cvetankanechevska Jul 15, 2022
8fdaca2
Return the value
cvetankanechevska Jul 18, 2022
2819de3
Add missing $
cvetankanechevska Jul 19, 2022
f935bc1
Merge branch 'main' into fix/reposition-loading-indicator
cvetankanechevska Jul 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion RELEASE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ Please follow the established format:

## Bug fixes and other changes

- General design-debt fixes. (#930, #955, #956, #959, #960)
- General design-debt fixes. (#930, #955, #956, #959, #960, #970)

# Release 4.7.1

Expand Down
12 changes: 10 additions & 2 deletions src/components/flowchart-wrapper/flowchart-wrapper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React from 'react';
import { connect } from 'react-redux';
import classnames from 'classnames';
import { isLoading } from '../../selectors/loading';
import ExportModal from '../export-modal';
import FlowChart from '../flowchart';
Expand All @@ -14,14 +15,20 @@ import './flowchart-wrapper.css';
* Main flowchart container. Handles showing/hiding the sidebar nav for flowchart view,
* the rendering of the flowchart, as well as the display of all related modals.
*/
export const FlowChartWrapper = ({ loading }) => (
export const FlowChartWrapper = ({ loading, sidebarVisible }) => (
<div className="kedro-pipeline">
<Sidebar />
<MetaData />
<div className="pipeline-wrapper">
<PipelineWarning />
<FlowChart />
<LoadingIcon className="pipeline-wrapper__loading" visible={loading} />
<div
className={classnames('pipeline-wrapper__loading', {
'pipeline-wrapper__loading--sidebar-visible': sidebarVisible,
})}
>
<LoadingIcon visible={true} />
</div>
</div>
<ExportModal />
<MetadataModal />
Expand All @@ -30,6 +37,7 @@ export const FlowChartWrapper = ({ loading }) => (

export const mapStateToProps = (state) => ({
loading: isLoading(state),
sidebarVisible: state.visible.sidebar,
});

export default connect(mapStateToProps)(FlowChartWrapper);
22 changes: 20 additions & 2 deletions src/components/flowchart-wrapper/flowchart-wrapper.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,30 @@
@import '../../styles/variables';
@import '../../styles/mixins';

$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;


.pipeline-wrapper {
height: 100%;
}

.pipeline-wrapper__loading {
align-items: center;
display: flex;
flex-direction: column;
height: 100%;
justify-content: center;
position: absolute;
right: 20px;
bottom: 20px;
top: 0;
transform: translateX($total-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

width: calc((100% - #{$total-sidebar-toolbar-width-closed}));

@media (min-width: $sidebar-width-breakpoint) {
&--sidebar-visible {
transform: translateX($total-sidebar-toolbar-width-open);
width: calc((100% - #{$total-sidebar-toolbar-width-open}));
}
}
}