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

StackAggregator - several fixes #3210

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

M-i-k-e-l
Copy link
Contributor

Description

Strictly speaking this is a breaking change (see change log notes), let me know what you think.

Although the previous code for the index comparison seemed correct, it did not work correctly on Android, the new logic works on both platforms (except on an edge case were the user is mixing controlled and uncontrolled usage).

For visibility I'll add the bug reproduction steps:

  1. Restart the StackAggregatorScreen.
  2. Press on the Open Stack button.
  3. Press on the Show less button.
  4. Press on the Close Stack button.
    Result - the edges of the cards "become straight" (this is actually the scale animation failing), I was not able to fix this and it seemed too small to invest more time into.

Changelog

🚫 StackAggregator - fix card being transparent on Android - BREAKING CHANGE - Cards' backgroundColor should now be passed via the backgroundColor prop and not via the contentContainerStyle prop
StackAggregator - onCollapseChanged will now be called after the animation has ended (as was intended)

Additional info

Ticket 4186

@Inbal-Tish
Copy link
Collaborator

@M-i-k-e-l I was wondering if there's a simpler way, like moving the 'backgroundColor: 'transparent' to the Animated.View and the contentContainerStyle to the Card's style. What do you think?

@M-i-k-e-l M-i-k-e-l assigned M-i-k-e-l and unassigned Inbal-Tish Sep 4, 2024
@M-i-k-e-l
Copy link
Contributor Author

M-i-k-e-l commented Sep 11, 2024

@M-i-k-e-l I was wondering if there's a simpler way, like moving the 'backgroundColor: 'transparent' to the Animated.View and the contentContainerStyle to the Card's style. What do you think?

It causes another bug when adding a card.

Marking with v8

@Inbal-Tish
Copy link
Collaborator

@M-i-k-e-l I was wondering if there's a simpler way, like moving the 'backgroundColor: 'transparent' to the Animated.View and the contentContainerStyle to the Card's style. What do you think?

It causes another bug when adding a card.

Marking with v8

Did you investigate the bug created when adding a card? Is it a complex fix?

@M-i-k-e-l
Copy link
Contributor Author

@M-i-k-e-l I was wondering if there's a simpler way, like moving the 'backgroundColor: 'transparent' to the Animated.View and the contentContainerStyle to the Card's style. What do you think?

It causes another bug when adding a card.
Marking with v8

Did you investigate the bug created when adding a card? Is it a complex fix?

If you mean the bug with the shadows then I've committed it already (sadly it did not work well in master).

@Inbal-Tish
Copy link
Collaborator

@M-i-k-e-l I was wondering if there's a simpler way, like moving the 'backgroundColor: 'transparent' to the Animated.View and the contentContainerStyle to the Card's style. What do you think?

It causes another bug when adding a card.
Marking with v8

Did you investigate the bug created when adding a card? Is it a complex fix?

If you mean the bug with the shadows then I've committed it already (sadly it did not work well in master).

Not what I meant. The shadow bug is on master. With my suggested fix there's a new bug where the cards disappear on iOS. I meant if you investigated this new one? Maybe it's a small fix?

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