Add FXIOS-10913 [Homepage] [Top Sites] full layout configuration for top sites #23918
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cyndichin
commented
Dec 20, 2024
| var numberOfTilesPerRow: Int? { | ||
| willSet { | ||
| guard newValue != numberOfTilesPerRow else { return } | ||
| queue.async { |
There was a problem hiding this comment.
I thought that dispatching actions were already only dispatching on main thread, so not sure why I need to add this, but I was getting a crash and ensuring the call happens on main thread, addresses the issue. Going to look at this again with fresh eyes on Monday, but opened a PR to review to get early eyes + if anyone has a clue.
cyndichin
force-pushed
the
cc/FXIOS-10913_add-number-of-rows-for-top-site-cal
branch
from
bdaf5e9 to
23cc16a
Compare
lmarceau
reviewed
Dec 20, 2024
cyndichin
force-pushed
the
cc/FXIOS-10913_add-number-of-rows-for-top-site-cal
branch
from
23cc16a to
fe685f6
Compare
cyndichin
force-pushed
the
cc/FXIOS-10913_add-number-of-rows-for-top-site-cal
branch
from
fe685f6 to
47c611f
Compare
Client.app: Coverage: 32.53
Generated by 🚫 Danger Swift against 47c611f |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📜 Tickets
Jira ticket
Github issue
💡 Description
Continues with implementing the rest of the logic for the layout configuration for the top sites section. See part I here.
This PR adds the ability to configure the number of rows shown for top sites section specified in the homepage settings.
topSitesData, but also new fieldsnumberOfRowsandnumberOfTilesPerRownumberOfRowsis set initially and updates when the user changes the configuration in settings, which is why we dispatch an action to update the state inTopSitesSettingsViewControllernumberOfTilesPerRowis updated when the value changes when being calculated inTopSitesDimensionImplementation, which is initialized inHomepageSectionLayoutProvider. Although it's a bit weird to have the layout update based on this and then use the layout configuration to update the data, I decided to go this route and follow what the existing logic is doing but simplifying and adopting Redux. This section may change as we want to revisit the whole logic in the future.numberOfTilesPerRowandnumberOfRows, we only get the first X amount of top data sites + update our datasource based on this calculationNote: I notice a discrepancy in the previous PR in that in landscape we are showing 7 tiles instead of 8. Most likely due to my change in using
environment.container.effectiveContentSizeinstead ofview.sizelike the previous code. Will create a separate ticket to address this.📝 Checklist
You have to check all boxes before merging
@Mergifyio backport release/v120)Screenshots
iPhone
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-12-20.at.11.28.48.mp4
iPad
ScreenRecording_12-20-2024.12-29-40_1.MP4