-
Notifications
You must be signed in to change notification settings - Fork 1.3k
For #27401 - refactor SelectableChip into Chip composable #27602
For #27401 - refactor SelectableChip into Chip composable #27602
Conversation
7e10099
to
a08cfbf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First round of feedback. Looking good so far!
app/src/main/java/org/mozilla/fenix/home/pocket/PocketStoriesComposables.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/mozilla/fenix/home/pocket/PocketStoriesComposables.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/mozilla/fenix/home/pocket/PocketStoriesComposables.kt
Outdated
Show resolved
Hide resolved
a08cfbf
to
010183f
Compare
This pull request has conflicts when rebasing. Could you fix it @HarrisonOg? 🙏 |
3d2a773
to
c3a2a5b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is looking good from my quick glance, but I think we're still in the draft phase even though I got the review ping. There's some code polish that needs to be done before I think a full review in earnest should be done. My recommendation is to look through the full changes yourselves and essentially do a mini review of your own changes. I will naturally point out any inconsistencies, areas where either the naming or docs could be improved or code that could be simplified further.
Also, a note about squashing/amending the typography commit into the main commit. It's okay to do multi-commits when we want to split up complex things, but each commit should still try to resolve an issue instead of a fixup.
app/src/main/java/org/mozilla/fenix/home/pocket/PocketStoriesComposables.kt
Outdated
Show resolved
Hide resolved
dff2306
to
fecfde0
Compare
app/src/main/java/org/mozilla/fenix/home/pocket/PocketStoriesComposables.kt
Outdated
Show resolved
Hide resolved
4c0b4c1
to
737eb89
Compare
737eb89
to
26eab78
Compare
5e3e016
to
78bb387
Compare
@MozillaNoah I tried to fix the UI test but I'm marking it as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 quick nits. I unfortunately also discovered that the original component used to style the Pocket categories was for iOS and neither the Chip nor square Chip in the design system match up. I'm going to start a conversation with UX to align on this. This has to be blocked until that's resolved 😞
app/src/androidTest/java/org/mozilla/fenix/ui/HomeScreenTest.kt
Outdated
Show resolved
Hide resolved
5b4d5f9
to
3ccc21c
Compare
5287a4e
to
389aac6
Compare
94c846e
to
3824920
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last nit, but this looks good to me! 🎉
a9151a4
to
dbda84d
Compare
dbda84d
to
fb12e4f
Compare
Pull Request checklist
QA
To download an APK when reviewing a PR (after all CI tasks finished running):
Checks
at the top of the PR page.firefoxci-taskcluster
group on the left to expand all tasks.build-debug
task.View task in Taskcluster
in the newDETAILS
section.GitHub Automation
Fixes #27401