Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

For #26190 - Update jump back in cfr logic and message #26203

Merged
merged 1 commit into from
Aug 26, 2022

Conversation

Alexandru2909
Copy link
Contributor

@Alexandru2909 Alexandru2909 commented Jul 27, 2022

Updated message and logic for jump back in onboarding message.

Pull Request checklist

  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

QA

  • QA Needed

To download an APK when reviewing a PR (after all CI tasks finished running):

  1. Click on Checks at the top of the PR page.
  2. Click on the firefoxci-taskcluster group on the left to expand all tasks.
  3. Click on the build-debug task.
  4. Click on View task in Taskcluster in the new DETAILS section.
  5. The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

Fixes #26190

@Alexandru2909 Alexandru2909 added the 🙅 waiting Issues that are blocked or has dependencies that are not ready label Jul 27, 2022
@Alexandru2909 Alexandru2909 force-pushed the 26190 branch 3 times, most recently from a875c5b to f92c905 Compare August 18, 2022 14:11
@Alexandru2909 Alexandru2909 removed the 🙅 waiting Issues that are blocked or has dependencies that are not ready label Aug 18, 2022
@Alexandru2909 Alexandru2909 marked this pull request as ready for review August 18, 2022 14:11
@Alexandru2909 Alexandru2909 requested review from a team as code owners August 18, 2022 14:11
@mozilla-mobile mozilla-mobile deleted a comment from mergify bot Aug 18, 2022
@@ -22,8 +22,9 @@
android:layout_marginTop="16dp"
android:layout_marginEnd="8dp"
android:layout_marginBottom="16dp"
android:maxWidth="200dp"
android:text="@string/onboarding_home_screen_jump_back_contextual_hint"
android:maxWidth="279dp"
Copy link
Contributor

Choose a reason for hiding this comment

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

where is this number derived from?

Copy link
Contributor Author

@Alexandru2909 Alexandru2909 Aug 18, 2022

Choose a reason for hiding this comment

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

For the maxWidth and maxHeight values, my intention was to respect the Figma values.

Here are a couple of screenshots:

Current After
before after

Should we stick with the maxWidth = 200dp and wrap_content height instead? We could also file a ticket to use the compose CFR to achieve this behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the height, I'd normally lean towards wrap_content so text can expand on small devices or devices with larger fonts without being cut-off.

For the width, I'd double check with UX to see if this is supposed to be a fixed width or wrap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tagging @Kate-Galetski to check whether the CFR should respect the design's width (see After picture in the previous comment) or the current CFR width.

Choose a reason for hiding this comment

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

Hi @Alexandru2909 the width looks fine to me! (atm we don't have any strict rules to how much % of the page the CFR should expand to.) What doesn't look fine to me is that this CFR is glued to the left border of the screen, while it actually should have a padding of 12px in there. Link to figma
@gabrielluong do we need a separate ticket for that adjustment as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems like a minor change so I added it in this ticket. In the figma designs, Android code tool indicates the value as dp, so here is an updated screenshot:
Screenshot_20220824_223841

If this should be addressed in another ticket we could also maintain the current maxWidth and update it in the CFR design issue.

Copy link
Member

Choose a reason for hiding this comment

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

I am accepting these values since Kate has ok'd them. However, I would ask that you test this with a different language (say German) to see if we need any further followup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested with multiple languages and the CFR looks fine, apart from the rtl languages, as they use the end padding and make the CFR look glued to the left edge of the screen. For this I added the 12dp padding to the end too.

@gabrielluong
Copy link
Member

Please test the CFR to see how it looks like with a different language (ex, German) prior to landing.

@gabrielluong gabrielluong added pr:approved PR that has been approved and removed needs:review PRs that need to be reviewed labels Aug 25, 2022
@mozilla-mobile mozilla-mobile deleted a comment from mergify bot Aug 25, 2022
@mergify
Copy link
Contributor

mergify bot commented Aug 25, 2022

This pull request has conflicts when rebasing. Could you fix it @Alexandru2909? 🙏

@Alexandru2909 Alexandru2909 added the pr:needs-landing PRs that are ready to land [Will be merged by Mergify] label Aug 26, 2022
@mergify mergify bot merged commit 21d3d0f into mozilla-mobile:main Aug 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr:approved PR that has been approved pr:needs-landing PRs that are ready to land [Will be merged by Mergify]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update jump back in onboarding message logic and message
4 participants