Skip to content

Conversation

@pubiqq
Copy link
Contributor

@pubiqq pubiqq commented Jun 12, 2023

No description provided.

@pubiqq pubiqq force-pushed the carousel/fix-on-size-changed branch from 6039806 to df8a325 Compare June 27, 2023 23:54
@imhappi
Copy link
Contributor

imhappi commented Jun 29, 2023

@hunterstich could you take a look to see if this was already addressed in your other change?

@imhappi imhappi requested a review from hunterstich June 29, 2023 20:12
@hunterstich
Copy link
Contributor

Hi @pubiqq,

Thanks for the fix! I actually have a change that's in review currently that addresses this issue along with some catalog issues in a slightly different way. Do you mind if I close this and use the change that's coming through internally?

@pubiqq
Copy link
Contributor Author

pubiqq commented Jul 7, 2023

👌

@pubiqq
Copy link
Contributor Author

pubiqq commented Jul 20, 2023

Looks like 14023d2 was rolled back by 64b066a. Would you mind reopening and re-reviewing my PR?

@dsn5ft
Copy link
Contributor

dsn5ft commented Aug 9, 2023

@hunterstich @imhappi any status update here?

@dsn5ft dsn5ft requested a review from imhappi August 9, 2023 17:16
@imhappi
Copy link
Contributor

imhappi commented Aug 9, 2023

I took a look at the PR, heads up that maskXPercentage is only set in setMaskXPercentage which is deprecated and not being used by any of our code. I've updated the PR to only update the mask rect with the mask x percentage if it was set in case someone is using the deprecated method, will send it out for review.

The mask rect actually gets directly set by the CarouselLayoutManager; we may have to make some other changes to let the CLM recalculate its keylines/masks in the onLayoutChildren method which should get called if the MaskableFrameLayout size is changed so I'll re-open #3490 but close this PR once it gets reviewed

@pubiqq pubiqq force-pushed the carousel/fix-on-size-changed branch from df8a325 to 6a7b506 Compare August 14, 2023 18:05
@pubiqq
Copy link
Contributor Author

pubiqq commented Aug 14, 2023

I created this PR when setMaskXPercentage was not deprecated yet. If you think it doesn't need fixing, you can just close the PR.

@imhappi imhappi closed this in dc91b39 Aug 18, 2023
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.

5 participants