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

[KED-2385] Refactor row toggle/label CSS, and fix row hover icon bug #358

Merged
merged 6 commits into from
Jan 27, 2021

Conversation

richardwestenra
Copy link
Contributor

@richardwestenra richardwestenra commented Jan 26, 2021

Description

  1. Instead of having just the _row.scss file and then filter overrides in _filter.scss, instead create one file for all the text label scss (both toggle/filter kinds), and one for the toggle-icon scss (both kinds)
  2. Fix toggle-icon--kind-filter parent row-hover bug: The toggle-all icon would decrease in opacity on row hover.
  3. Rename node-list row toggle --kind-toggle to --kind-element to make the distinction between the two elements a bit more clear. We can rename them again later if necessary but I think this makes things a lot easier to understand.
  4. Refactor node-list group creation selector to improve readability

Development notes

I discussed the naming of the two node-list row item kinds with @bru5 at length while working on this PR. We want to strike a balance between intuitiveness and extensibility, which is why we've avoided being too specific in the names of the two kinds (e.g. calling filters 'category', which might not work with future usages, or calling them 'blue-dot' and 'eye-icon', which is too dependent on the presentation).

QA notes

Please double-check the node-list hover/focus/selection states and see that nothing has changed (except for the blue dot checked parent row-hover state, which has been fixed).

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

Legal notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

  • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.

  • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.

  • I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

Instead of having just the _row.scss file and then filter overrides in _filter.scss, instead create one file for all the text label scss (both toggle/filter kinds), and one for the toggle-icon scss (both kinds)
The toggle-all icon would decrease in opacity on row hover. This commit fixes this
@richardwestenra richardwestenra self-assigned this Jan 26, 2021
@richardwestenra richardwestenra changed the title Refactor row toggle/label CSS, and fix row hover icon bug [KED-2385] Refactor row toggle/label CSS, and fix row hover icon bug Jan 26, 2021
This should make the two node-row items a bit easier to identify.
@richardwestenra richardwestenra marked this pull request as ready for review January 26, 2021 10:55
@studioswong
Copy link
Contributor

Thanks for the refactor, code looks good to me 👍

One thing to confirm - I notice that you mentioned that the 'toggle-all' icon would decrease in opacity on hover, however it looks like it is currently set at opacity 0 and only shows up on hover of the toggle button ( see below), which is very different from the existing behaviour of the filter all row on hover.
toggle 1

This is what we previously have:
toggle 2

Is this new change something intended, as it seems to deviate from the behaviour of the toggle all icon for the eye ( where the 'toggle-all' eye icon would still show up on toggle of the entire row, and only increase in opacity on hover of the toggle button?) @GabrielComymQB

The row hover effect would not show up on the parent when unchecked. I've reverted the previous fix and instead have ensured that the --checked modifier rule overrides the hover rule
@richardwestenra
Copy link
Contributor Author

@studioswong Good catch, thanks! 🙌

Fixed 😅

position: absolute;
top: 0;
bottom: 0;
left: -100px;
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: I noticed that we use em in some places ( for fonts and margins) and px in others, do we normally have any preference on which to use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would typically use em where we want it to be related/relative to font-size, and px where it should be absolute.

Copy link
Contributor

@studioswong studioswong left a comment

Choose a reason for hiding this comment

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

thanks for fixing - LGTF 👍

@richardwestenra richardwestenra merged commit 6d5b64b into main Jan 27, 2021
@richardwestenra richardwestenra deleted the refactor/node-list-css branch January 27, 2021 12:02
@richardwestenra richardwestenra mentioned this pull request Feb 17, 2021
1 task
richardwestenra added a commit that referenced this pull request Feb 19, 2021
# Release 3.9.0

## Major features and improvements

- Add code panel (#346)
- Improve view panning behaviour when a node is selected (#356, #363, #370, #373, #374)
- Improve layout performance for large graphs (#343)
- Save tag state to localStorage (#353)

## Bug fixes and other changes

- Improve graph layout code quality, performance and docs (#347)
- Update docs to remind on compatibility of Kedro-Viz 3.8.0 with Kedro 0.17 (#367)
- Update dependencies (#360, #364, #369)
- Fix failing CircleCI build on Windows (#365, #366)
- Refactor node-list-row CSS, fixing hover and focus states (#355, #358, #362)
- Update iconography (#357, #359)
- Fix missing indicator Chrome zoom bug (#349)
- Fix sidebar border/box-shadow CSS rules (#351)
- Fix server.py to work with versions >0.17 and update contributing docs (#348)
- Fix errors when scrolling with empty pipeline (#342)
- Ignore coverage on some branches and fix e2e tests (#345)
- Fix icon-button tooltips on mobile (#344)
- Update SVG-Crowbar to fix errors (#339)
- Update contributing docs for new dev server (#341)
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.

2 participants