Page MenuHomePhabricator

[native] Create muted tab tip
ClosedPublic

Authored by inka on Aug 14 2024, 7:06 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 12, 5:16 PM
Unknown Object (File)
Tue, Nov 12, 5:08 PM
Unknown Object (File)
Sat, Nov 9, 10:25 PM
Unknown Object (File)
Sat, Nov 9, 10:01 PM
Unknown Object (File)
Sat, Nov 9, 9:56 PM
Unknown Object (File)
Sat, Nov 9, 8:29 PM
Unknown Object (File)
Sun, Nov 3, 8:51 AM
Unknown Object (File)
Sat, Nov 2, 4:53 PM
Subscribers

Details

Summary

issue: ENG-8544

Test Plan

Please see the video:

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Aug 14 2024, 7:10 AM
Harbormaster failed remote builds in B31102: Diff 43388!
Harbormaster returned this revision to the author for changes because remote builds failed.Aug 14 2024, 7:52 AM
Harbormaster failed remote builds in B31104: Diff 43390!
inka requested review of this revision.Aug 14 2024, 8:18 AM
tomek added a reviewer: ashoat.
tomek added inline comments.
native/navigation/muted-tab-tip.react.js
27–32 ↗(On Diff #43391)

Memoizing simple strings usually isn't beneficial, but also shouldn't hurt.

native/navigation/muted-tab-tip.react.js
27–32 ↗(On Diff #43391)

And I think it looks much better when the string is a result of a function call basically.
const str = if(condition){return 5;}else{return 10;} is much better than
let str = 10; if(condition){str=5;}.

native/navigation/muted-tab-tip.react.js
34–38 ↗(On Diff #43391)

It is a component that has the type ComponentType.

ashoat requested changes to this revision.Aug 20 2024, 12:45 PM

The video in the Test Plan doesn't seem to show the right text. Can you share a screenshot of just mutedTabTipTextBase, and both mutedTabTipTextBase + mutedTabTipTextIfFID?

This revision now requires changes to proceed.Aug 20 2024, 12:45 PM

Here are screenshots with updated styles according to what we discussed in ENG-9058

image.png (2×1 px, 318 KB)

image.png (2×1 px, 327 KB)

This revision is now accepted and ready to land.Aug 21 2024, 12:25 AM

Add MutedTabTipRouteName to newReanimatedRoutes

Update approach due to changes requested in D13082.

inka requested review of this revision.Aug 27 2024, 6:31 AM

Requesting review because the changes are significant

I don't see a better way of doing this. The styles in materiar-top-tabs lib are inlined in MaterialTopTabBar.

New video:

(light mode drawer tip looks a bit broken, fixed in D13176

tomek added inline comments.
native/navigation/muted-tab-tip.react.js
22 ↗(On Diff #43679)

Is it intentional to have a leading white space?

This revision is now accepted and ready to land.Aug 27 2024, 7:38 AM
native/navigation/muted-tab-tip.react.js
22 ↗(On Diff #43679)

Yes! We don't want a trailing white space in the previous string, since it might be displayed alone. But this string is always concatenated with the first. So it includes the white space that separates the sentences

This revision was automatically updated to reflect the committed changes.