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

Fix/tags active inactive behaviour #939

Merged
merged 11 commits into from
Jun 28, 2022

Conversation

cvetankanechevska
Copy link
Contributor

@cvetankanechevska cvetankanechevska commented Jun 23, 2022

Description

Resolves #920

Development notes

Added new check if items list is empty or not, according to that toggle button will behave properly. And text label for that row will be displayed with less opacity.

Here we have disabled button for Tags:

Screenshot 2022-06-23 at 14 08 46

QA notes

Tests that we already have for node-list-groups and node-list-group are modified.

  • In node-list-groups - I am checking if DOM element has a disabled class for the button, if not we can than simulate click() event.
  • In node-list-group - I've added empty array as in mock data from spaceflights we don't have items data

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

@comym
Copy link

comym commented Jun 23, 2022

Hi @cvetankanechevska , I just had a look and it looks like you've built it correctly as the design reference. However, one thing that I just noticed is that, logically we could also make sure that, it the tags subgroup is disabled - as you're already showing - it would make more sense that the arrow indicator in the cell shows to be in it's "closed" position.

What do you think?

At the moment we are showing it like this, with the arrow pointing down.

Screenshot 2022-06-23 at 13 45 35

The idea is to instead, make it point to the left as if it's closed. Like, "this is disabled, closed, nothing to see here" :)

Screenshot 2022-06-23 at 13 46 18

Would it be ok to tweak it this way? Many thanks

@cvetankanechevska
Copy link
Contributor Author

Hi @cvetankanechevska , I just had a look and it looks like you've built it correctly as the design reference. However, one thing that I just noticed is that, logically we could also make sure that, it the tags subgroup is disabled - as you're already showing - it would make more sense that the arrow indicator in the cell shows to be in it's "closed" position.

What do you think?

At the moment we are showing it like this, with the arrow pointing down.

Screenshot 2022-06-23 at 13 45 35

The idea is to instead, make it point to the left as if it's closed. Like, "this is disabled, closed, nothing to see here" :)

Screenshot 2022-06-23 at 13 46 18

Would it be ok to tweak it this way? Many thanks

Yes, I agree with you. Will make the change.

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 great, and thanks for sorting some props. I'm approving with one thought:

I'd suggest you add a test to check that when items is 0 the new class name (pipeline-type-group-toggle--disabled) is actually set. I don't think you're doing that at the moment, are you?

@cvetankanechevska
Copy link
Contributor Author

Looks great, and thanks for sorting some props. I'm approving with one thought:

I'd suggest you add a test to check that when items is 0 the new class name (pipeline-type-group-toggle--disabled) is actually set. I don't think you're doing that at the moment, are you?

No, I don't have that check. I will try to add it.

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

@cvetankanechevska cvetankanechevska merged commit 40c70f6 into main Jun 28, 2022
@cvetankanechevska cvetankanechevska deleted the fix/tags-active-inactive-behavior branch June 28, 2022 13:40
@tynandebold tynandebold mentioned this pull request Jul 1, 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.

Improve ‘Tags’ active/inactive component behavior
4 participants