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

Refactor / node list row #2143

Merged
merged 47 commits into from
Oct 29, 2024
Merged
Show file tree
Hide file tree
Changes from 46 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
63431e4
update classnames to match the component name
Oct 15, 2024
e1ab740
update names in tests
Oct 15, 2024
976ff98
update the rest of the classnames
Oct 15, 2024
e5350ad
abstract node-list-row-toggle component
Oct 15, 2024
988ca60
tidy up code for toggle component
Oct 15, 2024
5d37a5b
update classnames in tests
Oct 16, 2024
abf00c8
simplify the css
Oct 16, 2024
3c2d623
add tests for node-list-row-toggle
Oct 16, 2024
479701e
remove handleToggle on VisibilityIcon
Oct 16, 2024
39c8db8
remove redux from node-list-row
Oct 16, 2024
ca8b72c
split node-list-row into row and filter-row
Oct 17, 2024
65382f8
rename toggle icon component
Oct 17, 2024
9162c10
move row and filter-row to components level
Oct 17, 2024
b174662
move css to row and filterRow
Oct 17, 2024
d87c2b2
remove node-list-row
Oct 17, 2024
a7ef9b6
separate the row-text component
Oct 17, 2024
433d222
include parent classname
Oct 17, 2024
c51deaa
update name for toggle-icon, to visibility-control
Oct 17, 2024
3774aec
fix css and move nodeListRowHeight to config
Oct 17, 2024
9c341ab
adding test for new component
Oct 18, 2024
46830b8
update classname for tests
Oct 18, 2024
c9bd1bb
move row inside node-list
Oct 22, 2024
80428af
connect redux store to component
Oct 22, 2024
55f6e3f
fix styling
Oct 22, 2024
d6e493e
update name to ToggleControl
Oct 22, 2024
b9fb79b
remove disable props as no longer needed
Oct 22, 2024
01f2498
replace js code with css to simplify the code
Oct 23, 2024
864b6cd
update classnames in cypress test
Oct 23, 2024
96b9199
Styling for hovering and focus mode
Oct 23, 2024
ab40577
fixing small styling
Oct 23, 2024
6089867
fix the disable styling for row
Oct 23, 2024
d108bcf
fix the disable styling on focus mode
Oct 24, 2024
29ded20
remove one of the old test
Oct 24, 2024
c539442
update name for icons for FilterRow
Oct 24, 2024
cdd10f1
fixing the icon highlighting issue
Oct 28, 2024
d3c16ae
remove un-used li element
Oct 28, 2024
3da4864
remove styling for pipeline-nodelist__placeholder-upper and lower cla…
Oct 28, 2024
db9a947
update test in node-list
Oct 28, 2024
e1c2dff
update cypress tests
Oct 29, 2024
8ef75b8
moving .pipeline-nodelist__group--all-unchecked to the parent
Oct 29, 2024
0abbf59
prevent page reload on form submission
Oct 29, 2024
db25f03
remove wrong classname in the test
Oct 29, 2024
cb342b5
remove unique ID
Oct 29, 2024
7891403
Merge branch 'main' into refactor/node-list-row
Oct 29, 2024
faac318
apply hovering styling on the parent instead of row
Oct 29, 2024
932b89a
styling for selected element
Oct 29, 2024
aaf9172
fixing hover styling on the icon from MUI
Oct 29, 2024
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 cypress/tests/ui/flowchart/flowchart.cy.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ describe('Flowchart DAG', () => {
const nodeToToggleText = 'Parameters';

// Alias
cy.get(`.pipeline-nodelist__row__checkbox[name=${nodeToToggleText}]`).as(
cy.get(`.toggle-control__checkbox[name=${nodeToToggleText}]`).as(
'nodeToToggle'
);

Expand Down
34 changes: 17 additions & 17 deletions cypress/tests/ui/flowchart/menu.cy.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ describe('Flowchart Menu', () => {
});

// Pipeline Label in the Menu
cy.get('.pipeline-nodelist__row__label')
cy.get('.row-text__label')
.first()
.invoke('text')
.should((pipelineLabel) => {
Expand All @@ -57,7 +57,7 @@ describe('Flowchart Menu', () => {
cy.get('.search-input__field').type(searchInput, { force: true });

// Pipeline Label in the Menu
cy.get('.pipeline-nodelist__row__label')
cy.get('.row-text__label')
.first()
.invoke('text')
.should((pipelineLabel) => {
Expand All @@ -72,7 +72,7 @@ describe('Flowchart Menu', () => {

// Action
cy.get(
`.MuiTreeItem-label > .pipeline-nodelist__row > [data-test=nodelist-data-${nodeToClickText}]`
`.MuiTreeItem-label > .node-list-tree-item-row > [data-test=node-list-tree-item--row--${nodeToClickText}]`
)
.should('exist')
.as('nodeToClick');
Expand All @@ -91,7 +91,7 @@ describe('Flowchart Menu', () => {

// Action
cy.get(
`.MuiTreeItem-label > .pipeline-nodelist__row > [data-test=nodelist-data-${nodeToHighlightText}]`
`.MuiTreeItem-label > .node-list-tree-item-row > [data-test=node-list-tree-item--row--${nodeToHighlightText}]`
)
.should('exist')
.as('nodeToHighlight');
Expand All @@ -108,7 +108,7 @@ describe('Flowchart Menu', () => {
const nodeToToggleText = 'Companies';

// Alias
cy.get(`.pipeline-nodelist__row__checkbox[name=${nodeToToggleText}]`, {
cy.get(`.toggle-control__checkbox[name=${nodeToToggleText}]`, {
timeout: 5000,
}).as('nodeToToggle');

Expand All @@ -121,7 +121,7 @@ describe('Flowchart Menu', () => {

// Assert after action
cy.__checkForText__(
`[data-test=nodelist-data-${nodeToToggleText}] > .pipeline-nodelist__row__label--faded`,
`[data-test=node-list-tree-item--row--${nodeToToggleText}] > .row-text__label--faded`,
nodeToToggleText
);
cy.get('.pipeline-node__text').should('not.contain', nodeToToggleText);
Expand All @@ -137,7 +137,7 @@ describe('Flowchart Menu', () => {

// Action
cy.get(
`[for=${nodeToFocusText}-focus] > .pipeline-nodelist__row__icon`
`[for=feature_engineering-focus]`
).click();

// Assert after action
Expand All @@ -161,34 +161,34 @@ describe('Flowchart Menu', () => {
const visibleRowLabel = 'Companies';

// Alias
cy.get(`.pipeline-nodelist__row__checkbox[name=${nodeToToggleText}]`).as(
cy.get(`.toggle-control__checkbox[name=${nodeToToggleText}]`).as(
'nodeToToggle'
);

// Assert before action
cy.get('@nodeToToggle').should('be.checked');
cy.get(
`[data-test=nodelist-data-${visibleRowLabel}] > .pipeline-nodelist__row__label`
`[data-test=node-list-tree-item--row--${visibleRowLabel}] > .row-text__label`
)
.should('not.have.class', 'pipeline-nodelist__row__label--faded')
.should('not.have.class', 'pipeline-nodelist__row__label--disabled');
.should('not.have.class', 'row-text__label--faded')
.should('not.have.class', 'row-text__label--disabled');

// Action
cy.get('@nodeToToggle').uncheck({ force: true });

// Assert after action
cy.get(
`[data-test=nodelist-data-${visibleRowLabel}] > .pipeline-nodelist__row__label`
`[data-test=node-list-tree-item--row--${visibleRowLabel}] > .row-text__label`
)
.should('have.class', 'pipeline-nodelist__row__label--faded')
.should('have.class', 'pipeline-nodelist__row__label--disabled');
.should('have.class', 'row-text__label--faded')
.should('have.class', 'row-text__label--disabled');
});

it('verifies that after checking node type URL should be updated with correct query params', () => {
const nodeToToggleText = 'Parameters';

// Alias
cy.get(`.pipeline-nodelist__row__checkbox[name=${nodeToToggleText}]`).as(
cy.get(`.toggle-control__checkbox[name=${nodeToToggleText}]`).as(
'nodeToToggle'
);

Expand All @@ -207,7 +207,7 @@ describe('Flowchart Menu', () => {
cy.visit(`/?tags=${visibleRowLabel}`);

// Alias
cy.get(`.pipeline-nodelist__row__checkbox[name=${visibleRowLabel}]`).as(
cy.get(`.toggle-control__checkbox[name=${visibleRowLabel}]`).as(
'nodeToToggle'
);

Expand All @@ -220,7 +220,7 @@ describe('Flowchart Menu', () => {
cy.visit('/?types=datasets');

// Alias
cy.get(`.pipeline-nodelist__row__checkbox[name=${visibleRowLabel}]`).as(
cy.get(`.toggle-control__checkbox[name=${visibleRowLabel}]`).as(
'nodeToToggle'
);

Expand Down
8 changes: 4 additions & 4 deletions cypress/tests/ui/toolbar/global-toolbar.cy.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,14 @@ describe('Global Toolbar', () => {
cy.get('@isPrettyNameCheckbox').should('be.checked');

// Menu
cy.get(`[data-test="nodelist-modularPipeline-${prettifyName(modularPipelineText)}"]`).click();
cy.get(`[data-test="nodelist-${nodeNameType}-${prettyNodeNameText}"]`).should('exist');
cy.get(`[data-test="node-list-tree-item--row--${prettifyName(modularPipelineText)}"]`).click();
cy.get(`[data-test="node-list-tree-item--row--${prettyNodeNameText}"]`).should('exist');

// Flowchart
cy.get('.pipeline-node__text').should('contain', prettyNodeNameText);

// Metadata
cy.get(`[data-test="nodelist-${nodeNameType}-${prettyNodeNameText}"]`).click({ force: true });
cy.get(`[data-test="node-list-tree-item--row--${prettyNodeNameText}"]`).click({ force: true });
cy.get('.pipeline-metadata__title').should(
'have.text',
prettyNodeNameText
Expand All @@ -106,7 +106,7 @@ describe('Global Toolbar', () => {
// Assert after action
cy.__waitForPageLoad__(() => {
// Menu
cy.get(`[data-test="nodelist-${nodeNameType}-${originalNodeNameText}"]`).should('exist');
cy.get(`[data-test="node-list-tree-item--row--${originalNodeNameText}"]`).should('exist');

// Flowchart
cy.get('.pipeline-node__text').should('contain', originalNodeNameText);
Expand Down
66 changes: 66 additions & 0 deletions src/components/filter-row/filter-row.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import React from 'react';
import classnames from 'classnames';
import IndicatorIcon from '../icons/indicator';
import OffIndicatorIcon from '../icons/indicator-off';
import { ToggleControl } from '../ui/toggle-control/toggle-control';
import { RowText } from '../ui/row-text/row-text';

import './filter-row.scss';

export const FilterRow = ({
allUnchecked,
Copy link
Contributor

Choose a reason for hiding this comment

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

there's some problem here, when I uncheck everything, the visibility icons disappear.

Copy link
Contributor Author

@Huongg Huongg Oct 24, 2024

Choose a reason for hiding this comment

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

it should be fixed now, the PR is ready for reviewing the UI parts now 😄

checked,
children,
container: ContainerWrapper,
count,
dataTest,
id,
indicatorIcon = IndicatorIcon,
kind,
label,
name,
offIndicatorIcon = OffIndicatorIcon,
onChange,
onClick,
parentClassName,
visible,
}) => {
const Icon = checked ? indicatorIcon : offIndicatorIcon;

return (
<ContainerWrapper
className={classnames(
'filter-row kedro',
`filter-row--kind-${kind}`,
parentClassName,
{
'filter-row--visible': visible,
'filter-row--unchecked': !checked,
}
)}
title={name}
>
<RowText
kind={kind}
dataTest={dataTest}
onClick={onClick}
name={children ? null : name}
label={label}
/>
<span onClick={onClick} className={'filter-row__count'}>
{count}
</span>
<ToggleControl
allUnchecked={allUnchecked}
className={'filter-row__icon'}
IconComponent={Icon}
id={id}
isChecked={checked}
kind={kind}
name={name}
onChange={onChange}
/>
{children}
</ContainerWrapper>
);
};
54 changes: 54 additions & 0 deletions src/components/filter-row/filter-row.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
@use '../../styles/variables' as var;
@use '../node-list/styles/variables';

.MuiTreeItem-iconContainer svg {
z-index: var.$zindex-MuiTreeItem-icon;
}

.filter-row {
align-items: center;
background-color: initial;
cursor: default;
display: flex;
height: 32px;
position: relative;

&--kind-filter {
padding: 0 variables.$row-offset-right 0 variables.$row-offset-left;
}

&--visible:hover {
background-color: var(--color-nodelist-row-active);
}
}

.filter-row__count {
display: inline-block;
flex-shrink: 0;
width: 2.2em;
margin: 0 0.7em 0.1em auto;
overflow: hidden;
font-size: 1.16em;
text-align: right;
text-overflow: ellipsis;
opacity: 0.75;
user-select: none;

.filter-row--unchecked & {
opacity: 0.55;
}
}

.filter-row--unchecked {
// Fade row text when unchecked
.row-text__label--kind-filter {
opacity: 0.55;
}

// Brighter row text when unchecked and hovered
&:hover {
.row-text__label--kind-filter {
opacity: 0.8;
}
}
}
24 changes: 24 additions & 0 deletions src/components/filter-row/filter-row.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import React from 'react';
import { mount } from 'enzyme';
import { FilterRow } from './filter-row';

describe('FilterRow Component', () => {
it('renders without crashing', () => {
const wrapper = mount(<FilterRow container={'div'} />);
expect(wrapper.exists()).toBe(true);
});

it('renders correct visible classnames', () => {
const wrapper = mount(<FilterRow container={'div'} visible={true} />);
expect(wrapper.find('.filter-row').hasClass('filter-row--visible')).toBe(
true
);
});

it('renders correct unchecked classnames', () => {
const wrapper = mount(<FilterRow container={'div'} checked={false} />);
expect(wrapper.find('.filter-row').hasClass('filter-row--unchecked')).toBe(
true
);
});
});
Loading
Loading