Fix Terms And Licenses text styles#7483
Conversation
Use <Text> parent element so text wrapping occurs "naturally"
MelvinBot
requested review from
luacmartins and
parasharrajat
and removed request for
a team
|
Not sure what's causing the checks to fail, I branched of Error: .github/workflows/e2e.yml (Line: 56, Col: 16):
Error: The template is not valid. .github/workflows/e2e.yml (Line: 56, Col: 16): hashFiles('**/Podfile.lock') couldn't finish within 120 seconds. |
luacmartins
left a comment
luacmartins
left a comment
There was a problem hiding this comment.
I agree with Rajat's suggestions. Otherwise, code looks good.
I haven't seen that error before, but I'm asking around. For now, I'll restart the job to see if that fixes it.
parasharrajat
left a comment
parasharrajat
left a comment
There was a problem hiding this comment.
I think now we can combine some of the phrases as well. Such as phrases 6 and 7.
Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>
|
@parasharrajat I've accepted the suggestions but they resulted in unsigned commits Was I expected to apply them manually? I see there are some lint issues as well, so I'll submit another commit |
|
Yeah Please fix the lint issues. Github gives limited options for code suggestions. But Code signing is fine and both are verified. As two users signed the commit it goes to vigilant mode. I don't know why it happens but GitHub does that. https://docs.github.com/github/authenticating-to-github/displaying-verification-statuses-for-all-of-your-commits. |
There was a problem hiding this comment.
LGTM.
A suggestion could be to merge the adjacent phrases. e.g.
{props.translate('termsOfUse.phrase5')}
{' '}
{props.translate('termsOfUse.phrase6')}
Leaving this suggestion to @luacmartins to decide.
🎀 👀 🎀 C+ reviewed
Yeah, it seems artificially split, should be no longer necessary, but I'm done for today |
|
Since we are touching this code, I think it would be a good cleanup to merge the adjacent phrases as suggested. Thanks for the suggestion @parasharrajat! |
|
Joined phrases 5 and 6 |
parasharrajat
left a comment
parasharrajat
left a comment
There was a problem hiding this comment.
LGTM.
cc: @luacmartins
🎀 👀 🎀 C+ reviewed
luacmartins
left a comment
luacmartins
left a comment
There was a problem hiding this comment.
LGTM! Thanks for the changes @kidroca!
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by @luacmartins in version: 1.1.33-4 🚀
|
|
@kidroca , Placeholder text and entered email are overlapping and not completely visible if the font size in the device is set to large. |
|
I think this is expected unless it only happens on this PR I suggested zooming too verify the text for Terms and Conditions would wrap correctly when it has too Maybe we should open a ticket about setting font size to large, but isn’t that covered under accessibility issues, e.g. existing ticket somewhere |
|
🚀 Deployed to production by @sketchydroide in version: 1.1.34-0 🚀
|
5 participants
Use parent element so text wrapping occurs "naturally"
Details
Fixed Issues
$ #7437
Tests
QA Steps
Same as above
Tested On
Screenshots
Web
No diff for English (staging vs local)
Spanish wrapping fixed
150% zoom (staging vs local)
Mobile Web
Desktop
iOS
Android