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(selection-panel): improved open/close flow and animation #2360

Merged
merged 20 commits into from
Feb 5, 2024

Conversation

TomMenga
Copy link
Contributor

@TomMenga TomMenga commented Jan 22, 2024

This PR aims to solve the problems relative to the open/close events emitted by the selection-panel.
We have received no complaints, so I expect nobody is using them so far.
Might solve #2309

Changes

  • Converted the open/close animation from css-transition to css-animation. CSS animation provides a more consistent behavior and always emits animationend events.
  • Refactored the code so that we have a single method that takes care of checking the state and to open/close the panel

To discuss

  • Should a panel that uses force-open emit events?
  • Should a panel that uses force-open animate?

@TomMenga TomMenga requested a review from dauriamarco January 22, 2024 10:49
@TomMenga TomMenga self-assigned this Jan 22, 2024
@github-actions github-actions bot requested a deployment to preview-pr2360 January 22, 2024 10:59 In progress
@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@ca278d5). Click here to learn what that means.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2360   +/-   ##
=======================================
  Coverage        ?   93.55%           
=======================================
  Files           ?      221           
  Lines           ?    23419           
  Branches        ?     2100           
=======================================
  Hits            ?    21909           
  Misses          ?     1474           
  Partials        ?       36           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot requested a deployment to preview-pr2360 January 22, 2024 11:13 In progress
@TomMenga TomMenga changed the title refactor(selection-panel): improved open/close flow and animation refactor(selection-panel): improved open/close flow and animation (WIP) Jan 22, 2024
@github-actions github-actions bot requested a deployment to preview-pr2360 January 22, 2024 13:12 In progress
@github-actions github-actions bot requested a deployment to preview-pr2360 January 22, 2024 15:27 In progress
@github-actions github-actions bot requested a deployment to preview-pr2360 January 22, 2024 16:24 In progress
@TomMenga TomMenga force-pushed the refactor/selection-panel-open branch from 7bcb42d to f5c032e Compare January 22, 2024 16:40
@github-actions github-actions bot requested a deployment to preview-pr2360 January 22, 2024 16:52 In progress
@github-actions github-actions bot requested a deployment to preview-pr2360 January 23, 2024 09:38 In progress
@TomMenga TomMenga changed the title refactor(selection-panel): improved open/close flow and animation (WIP) refactor(selection-panel): improved open/close flow and animation Jan 23, 2024
@github-actions github-actions bot requested a deployment to preview-pr2360 January 23, 2024 10:11 In progress
@TomMenga TomMenga changed the base branch from main to chore/selection-panel January 23, 2024 10:27
@TomMenga TomMenga force-pushed the refactor/selection-panel-open branch from 3b6aa91 to 626d133 Compare January 23, 2024 10:30
@github-actions github-actions bot requested a deployment to preview-pr2360 January 23, 2024 10:39 In progress
@github-actions github-actions bot requested a deployment to preview-pr2360 January 23, 2024 11:13 In progress
@github-actions github-actions bot requested a deployment to preview-pr2360 January 23, 2024 13:03 In progress
@github-actions github-actions bot requested a deployment to preview-pr2360 January 23, 2024 13:35 In progress
@TomMenga TomMenga changed the base branch from chore/selection-panel to main January 23, 2024 15:32
@github-actions github-actions bot requested a deployment to preview-pr2360 January 23, 2024 15:58 In progress
@github-actions github-actions bot requested a deployment to preview-pr2360 January 23, 2024 16:16 In progress
Copy link
Contributor

@jeripeierSBB jeripeierSBB left a comment

Choose a reason for hiding this comment

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

Now there is an initial animation which should not be there (compare to current version).

Should a panel that uses force-open emit events?
-> In my opinion yes, you don't have to listen to it if you don't care ;)
Should a panel that uses force-open animate?
-> no

src/components/selection-panel/selection-panel.scss Outdated Show resolved Hide resolved
@TomMenga
Copy link
Contributor Author

TomMenga commented Jan 25, 2024

Now there is an initial animation which should not be there (compare to current version).

Should a panel that uses force-open emit events? -> In my opinion yes, you don't have to listen to it if you don't care ;) Should a panel that uses force-open animate? -> no

I will clarify the current and past situation.

Before the update:

  • No animation on init (by looking at the code, it seemed not intentional but just a happy outcome)
  • force-open did animate (except if it was initialized with it)
  • events were always emitted (there were some broken cases where it would only emit the willOpen)

I suggest that we keep the same behavior, so:

  • No animation on init
  • force-open does animate (except if inited with it)
  • events always emitted

@jeripeierSBB what do you think?

@github-actions github-actions bot requested a deployment to preview-pr2360 January 25, 2024 10:37 In progress
@TomMenga TomMenga requested a review from jeripeierSBB January 25, 2024 10:52
Copy link
Contributor

@dauriamarco dauriamarco left a comment

Choose a reason for hiding this comment

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

LGTM. I'm just not really sure if we should animate the panel if we add the force-open, but I think it's just a small detail to be confirmed.

@jeripeierSBB
Copy link
Contributor

Now there is an initial animation which should not be there (compare to current version).
Should a panel that uses force-open emit events? -> In my opinion yes, you don't have to listen to it if you don't care ;) Should a panel that uses force-open animate? -> no

I will clarify the current and past situation.

Before the update:

  • No animation on init (by looking at the code, it seemed not intentional but just a happy outcome)
  • force-open did animate (except if it was initialized with it)
  • events were always emitted (there were some broken cases where it would only emit the willOpen)

I suggest that we keep the same behavior, so:

  • No animation on init
  • force-open does animate (except if inited with it)
  • events always emitted

@jeripeierSBB what do you think?

@TomMenga sounds good to me

Copy link
Contributor

@jeripeierSBB jeripeierSBB left a comment

Choose a reason for hiding this comment

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

LGTM. thanks!

@TomMenga TomMenga merged commit f0ad925 into main Feb 5, 2024
16 checks passed
@TomMenga TomMenga deleted the refactor/selection-panel-open branch February 5, 2024 13:59
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.

4 participants