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: withdrawn controls are not displayed correctly #827

Merged
merged 9 commits into from
May 3, 2023

Conversation

kylelaker
Copy link
Contributor

@kylelaker kylelaker commented Apr 28, 2023

This makes it so that withdrawn controls can be expanded. It also makes it so that nested controls/groups display as withdrawn if a parent is withdrawn.

  • Withdrawn Control
    image

  • Withdrawn Control with children
    image

@kylelaker kylelaker marked this pull request as ready for review May 2, 2023 16:40
Control lists will use the line-through text decoration when
withdrawn is true.
@Bronstrom
Copy link
Contributor

Bronstrom commented May 3, 2023

I'm noticing a few visual differences when testing this out:

First of all, I notice a few styling differences. There is no gap between out top-level controls, the control's are white (was a very light gray), and the corners are no longer rounded.

Before:

before_withdrawn

After:

after_withdrawn

Also, I see no click animation occurring on the top-level controls, and this may just be an issue on my end but when I open a control, the opening animation seems more sluggish.

@kylelaker
Copy link
Contributor Author

First of all, I notice a few styling differences. There is no gap between out top-level controls, the control's are white (was a very light gray), and the corners are no longer rounded.

This is intentional as part of the switch from a Collapse to an Accordion to match the other collapsable fields in the application. Is this an observation or a requested change?

@kylelaker
Copy link
Contributor Author

Also, I see no click animation occurring on the top-level controls, and this may just be an issue on my end but when I open a control, the opening animation seems more sluggish.

So are you seeing an animation or not? I am confused about what exactly you're reporting here.

Comment on lines 151 to 153
onEntered: () => !group && setListItemOpened(true),
unmountOnExit: true,
timeout: 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what we're using to prevent the long transitions. If you want to have animations but faster, then increase timeout. It is 0 because as-is on develop today, if you open AC-3 and then close it, you have to wait for the animation to close things below your viewport before you see any feedback that you clicked the button.

Maybe 0 is too jarring but suggest a value.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think 0 is fine for now. It is a pretty simple thing to change later if we want to.

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 changed this to 500

Copy link
Contributor

@tuckerzp tuckerzp left a comment

Choose a reason for hiding this comment

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

After playing with the accordion more, I do think this looks and works better than what we previously had.

Thanks for your work on this @kylelaker

Comment on lines 151 to 153
onEntered: () => !group && setListItemOpened(true),
unmountOnExit: true,
timeout: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think 0 is fine for now. It is a pretty simple thing to change later if we want to.

Copy link
Contributor

@Bronstrom Bronstrom left a comment

Choose a reason for hiding this comment

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

Looks good @kylelaker!

@kylelaker
Copy link
Contributor Author

@tuckerzp and @Bronstrom I have updated the styling to be closer to the old styling but also slightly closer to the Accordion examples, which should make this easier to later reuse in Metadata (I will open a PR for that after this one has been merged)

@Bronstrom Bronstrom merged commit 60e6cc2 into develop May 3, 2023
5 checks passed
@Bronstrom Bronstrom deleted the kyle/withdrawn-controls branch May 3, 2023 19:22
tuckerzp added a commit that referenced this pull request May 25, 2023
This makes it so that withdrawn controls can be expanded. It also makes
it so that nested controls/groups display as withdrawn if a parent is
withdrawn.

- Withdrawn Control 

![image](https://user-images.githubusercontent.com/850893/236020625-f7903096-5d8d-4c20-b607-2c660f37979e.png)


- Withdrawn Control with children 

![image](https://user-images.githubusercontent.com/850893/236020711-16e4fe73-de5d-4b3d-ad43-c9f54e893500.png)

---------

Co-authored-by: Zachary Tucker <ztucker@easydynamics.com>
@kylelaker kylelaker added the bug Something isn't working label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants