feat: horizontal section and label for Clear Signing #7529

RamyEB · 2024-08-06T14:59:50Z

✅ Checklist

npx changeset was attached.
Covered by automatic tests.
Impact of the changes:
- ...

📝 Description

Add clearsigning label to items in Web3Hub
Add horizontal section "Clear Signing"

❓ Context

JIRA or GitHub link: TICKET LIVE-13569

🧐 Checklist for the PR Reviewers

The code aligns with the requirements described in the linked JIRA or GitHub issue.
The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
There are no undocumented trade-offs, technical debt, or maintainability issues.
The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
Any new dependencies have been justified and documented.
Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

vercel · 2024-08-06T14:59:54Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

5 Skipped Deployments

Name	Status	Preview	Updated (UTC)
ledger-live-docs	⬜️ Ignored (Inspect)	Visit Preview	Aug 21, 2024 10:44am
ledger-live-github-bot	⬜️ Ignored (Inspect)	Visit Preview	Aug 21, 2024 10:44am
native-ui-storybook	⬜️ Ignored (Inspect)	Visit Preview	Aug 21, 2024 10:44am
react-ui-storybook	⬜️ Ignored (Inspect)	Visit Preview	Aug 21, 2024 10:44am
web-tools	⬜️ Ignored (Inspect)	Visit Preview	Aug 21, 2024 10:44am

Justkant

Please configure prettier to run correctly in your IDE to avoid this kind of issues

Justkant · 2024-08-07T08:13:31Z

apps/ledger-live-mobile/src/locales/en/common.json

+      "label": {
+        "clearSigning": "Clear signing"
+      }


Doesn't really make sense to have it in manifestList IMO

Justkant · 2024-08-07T08:17:23Z

...ger-live-mobile/src/newArch/features/Web3Hub/components/ManifestsList/ManifestItem/index.tsx

-              </Text>
-            )}
+                }),
+                getItemStyle(manifest.branch, colors)


Why would you call that again while we already call that in a memo above ?
I'd prefer to not confuse and merge a style based on the branch with a base style not based on it at all for another not related badge

apps/ledger-live-mobile/src/newArch/features/Web3Hub/components/Disclaimer/index.tsx

...ger-live-mobile/src/newArch/features/Web3Hub/components/ManifestsList/ManifestItem/index.tsx

...live-mobile/src/newArch/features/Web3Hub/components/ManifestsList/LoadingIndicator/index.tsx

Justkant · 2024-08-16T07:35:34Z

...ger-live-mobile/src/newArch/features/Web3Hub/components/ManifestsList/ManifestItem/index.tsx

  return (
    <TouchableOpacity disabled={isDisabled} onPress={handlePress}>
      <Flex flexDirection="row" alignItems="center" height={72} paddingX={4} paddingY={2}>
        <AppIcon isDisabled={isDisabled} size={48} name={manifest.name} icon={icon} />
        <Flex marginX={16} height="100%" flexGrow={1} flexShrink={1} justifyContent={"center"}>
-          <Flex flexDirection="row" alignItems={"center"} mb={2}>
+          <Flex flexDirection="row" alignItems={"center"} mb={2} columnGap={4}>


Not sure we'll go with columnGap while we use margins for everything else, and we'll probably remove or update the other because I don't think an experimental badge followed by a clear-signing one would probably take a lot of space on the line

Also columnGap below wouldn't add some margin left before the first badge so you instead have to add a columnGap above while it would not be needed with simpler margins

If the fact that margins are used for everything else is the main argument, then okay

Otherwise, I think gap is a better practice since we're using flex, and it's cleaner for managing spacing in this context. As we deal with flexbox and want to add spacing between items, it's better to avoid affecting the first or last element

Justkant · 2024-08-16T07:40:08Z

...Arch/features/Web3Hub/screens/Web3HubMain/components/HorizontalList/MinimalAppCard/index.tsx

+  return (
+    <TouchableOpacity disabled={item.branch === "soon"} onPress={onPress}>
+      <Flex rowGap={6} marginRight={3} width={70} alignItems={"center"}>
+        <AppIcon isDisabled={item.branch === "soon"} size={48} name={item.name} icon={item.icon} />
+        <Text numberOfLines={1}>{item.name}</Text>
+      </Flex>
+    </TouchableOpacity>
+  );


Small detail but we can probably create a const for disabled props

Suggested change

return (

<TouchableOpacity disabled={item.branch === "soon"} onPress={onPress}>

<Flex rowGap={6} marginRight={3} width={70} alignItems={"center"}>

<AppIcon isDisabled={item.branch === "soon"} size={48} name={item.name} icon={item.icon} />

<Text numberOfLines={1}>{item.name}</Text>

</Flex>

</TouchableOpacity>

);

const disabled = item.branch === "soon";

return (

<TouchableOpacity disabled={disabled} onPress={onPress}>

<Flex rowGap={6} marginRight={3} width={70} alignItems={"center"}>

<AppIcon isDisabled={disabled} size={48} name={item.name} icon={item.icon} />

<Text numberOfLines={1}>{item.name}</Text>

</Flex>

</TouchableOpacity>

);

Justkant · 2024-08-16T07:52:05Z

...-mobile/src/newArch/features/Web3Hub/screens/Web3HubMain/components/HorizontalList/index.tsx

+  extraData?: { onPress: (manifest: AppManifest) => void };
+};
+
+const identityFn = (item: AppManifest) => item.id.toString();


This should already be a string

Suggested change

const identityFn = (item: AppManifest) => item.id.toString();

const identityFn = (item: AppManifest) => item.id;

...-mobile/src/newArch/features/Web3Hub/screens/Web3HubMain/components/HorizontalList/index.tsx

Justkant · 2024-08-16T07:59:36Z

...-mobile/src/newArch/features/Web3Hub/screens/Web3HubMain/components/HorizontalList/index.tsx

+      <Text mt={2} mb={5} numberOfLines={1} variant="h5" mx={5} accessibilityRole="header">
+        {title}
+      </Text>
+      <View style={{ height: "auto", marginBottom: 2 }}>


height: "auto" should already be the default

Suggested change

<View style={{ height: "auto", marginBottom: 2 }}>

<View style={{ marginBottom: 2 }}>

We should then use the Box component to use the proper spacings of our design system

Suggested change

<View style={{ height: "auto", marginBottom: 2 }}>

<Box mb={2}>

Justkant · 2024-08-16T08:00:27Z

.../src/newArch/features/Web3Hub/screens/Web3HubMain/components/ManifestsCategoryList/index.tsx

+
+  return data && data.length > 0 ? (
+    <>
+      <Disclaimer disclaimer={disclaimer} />


I'll probably do a quick refactor of the disclaimer in web3hub after this PR

Justkant

As I said previously you should take as an example the ManifestItem code for your item logic around props and item press

Justkant · 2024-08-19T06:50:01Z

...-mobile/src/newArch/features/Web3Hub/screens/Web3HubMain/components/HorizontalList/index.tsx

+    const disabled = item.branch === "soon";
+    const handlePress = () => {
+      if (!disabled) {
+        extraData(item);
+      }
+    };


This logic should be in the component itself with a useCallback for the function too

Justkant · 2024-08-19T06:50:02Z

...-mobile/src/newArch/features/Web3Hub/screens/Web3HubMain/components/HorizontalList/index.tsx

+      }
+    };
+
+    return <MinimalAppCard key={item.id} disabled={disabled} item={item} onPress={handlePress} />;


The key prop is not needed with flashlist

Suggested change

return <MinimalAppCard key={item.id} disabled={disabled} item={item} onPress={handlePress} />;

return <MinimalAppCard disabled={disabled} item={item} onPress={handlePress} />;

Justkant · 2024-08-19T06:50:43Z

...-mobile/src/newArch/features/Web3Hub/screens/Web3HubMain/components/HorizontalList/index.tsx

@@ -34,12 +29,23 @@ export default function HorizontalList({
  extraData,
  testID,
 }: Props) {
+  const renderItem = useCallback(({ item, extraData = () => {} }: PropRenderItem) => {


You can move this outside the component

Justkant · 2024-08-19T06:52:48Z

.../src/newArch/features/Web3Hub/screens/Web3HubMain/components/ManifestsCategoryList/index.tsx

@@ -34,7 +34,7 @@ const ManifestsCategoryList = ({ title, categoryId, navigation }: Props) => {
      <HorizontalList
        title={title}
        data={data}
-        extraData={{ onPress: disclaimer.onPressItem }}
+        extraData={disclaimer.onPressItem}


This prop should probably be called onPressItem as we don't really want to say that we expose extraData especially because nothing else than a function would work here

Justkant · 2024-08-19T06:53:45Z

...-mobile/src/newArch/features/Web3Hub/screens/Web3HubMain/components/HorizontalList/index.tsx

 import MinimalAppCard from "./MinimalAppCard";

 type Props = {
  title: string;
  isLoading: boolean;
  data: AppManifest[];
  onEndReached?: () => void;
-  extraData: { onPress: (manifest: AppManifest) => void };
+  extraData: (manifest: AppManifest) => void;


Related to the other comment on this

Suggested change

extraData: (manifest: AppManifest) => void;

onPressItem: (manifest: AppManifest) => void;

RamyEB requested a review from a team as a code owner August 6, 2024 14:59

github-actions bot assigned RamyEB Aug 6, 2024

live-github-bot bot added mobile Has changes in LLM translations Translation files have been touched labels Aug 6, 2024

Justkant reviewed Aug 7, 2024

View reviewed changes

Justkant reviewed Aug 9, 2024

View reviewed changes

RamyEB changed the title ~~feat: add label~~ feat: horizontal section and label for Clear Signing Aug 12, 2024

RamyEB force-pushed the feat/LIVE-12743-add-clearsigning-label branch from ebf63e8 to c81d8c1 Compare August 13, 2024 14:26

Justkant reviewed Aug 16, 2024

View reviewed changes

Justkant reviewed Aug 19, 2024

View reviewed changes

RamyEB force-pushed the feat/LIVE-12743-add-clearsigning-label branch from a80e43c to 2200451 Compare August 19, 2024 09:58

Justkant previously approved these changes Aug 20, 2024

View reviewed changes

RamyEB added 14 commits August 20, 2024 17:17

feat: add label

2c7fdfe

add clear signing horizontal list

da22ad4

fix: prettier

a333ff6

fix: change translation location

c99a8e8

fix: put styles related to branch

7ff6cfa

fix: add memo and move function label to component

eac580b

refacto horizontal list

ac9b0ac

feat: add test

15d24f4

feat: fix after rebase

55fc6de

feat: fix after rebase

422ee33

fix: review correction

8744350

fix: review correction

8fcad2a

fix: lint correction

f130274

fix: review correction

0e2da57

RamyEB force-pushed the feat/LIVE-12743-add-clearsigning-label branch from 2200451 to 0e2da57 Compare August 20, 2024 15:17

fix: props wrongly passed to renderItem

4e294f8

RamyEB dismissed Justkant’s stale review via 4e294f8 August 21, 2024 10:43

Justkant approved these changes Aug 21, 2024

View reviewed changes

RamyEB merged commit a409ecc into develop Aug 21, 2024
35 of 37 checks passed

RamyEB deleted the feat/LIVE-12743-add-clearsigning-label branch August 21, 2024 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: horizontal section and label for Clear Signing #7529

feat: horizontal section and label for Clear Signing #7529

RamyEB commented Aug 6, 2024 •

edited by jira bot

Loading

vercel bot commented Aug 6, 2024 •

edited

Loading

Justkant left a comment •

edited

Loading

Justkant Aug 7, 2024

Justkant Aug 7, 2024

Justkant Aug 16, 2024

RamyEB Aug 16, 2024

Justkant Aug 16, 2024

Justkant Aug 16, 2024

Justkant Aug 16, 2024

Justkant Aug 16, 2024

Justkant left a comment

Justkant Aug 19, 2024

Justkant Aug 19, 2024

Justkant Aug 19, 2024

Justkant Aug 19, 2024

Justkant Aug 19, 2024

	const identityFn = (item: AppManifest) => item.id.toString();
	const identityFn = (item: AppManifest) => item.id;

	<View style={{ height: "auto", marginBottom: 2 }}>
	<View style={{ marginBottom: 2 }}>

	<View style={{ height: "auto", marginBottom: 2 }}>
	<Box mb={2}>

	return <MinimalAppCard key={item.id} disabled={disabled} item={item} onPress={handlePress} />;
	return <MinimalAppCard disabled={disabled} item={item} onPress={handlePress} />;

	extraData: (manifest: AppManifest) => void;
	onPressItem: (manifest: AppManifest) => void;

feat: horizontal section and label for Clear Signing #7529

feat: horizontal section and label for Clear Signing #7529

Conversation

RamyEB commented Aug 6, 2024 • edited by jira bot Loading

✅ Checklist

📝 Description

❓ Context

🧐 Checklist for the PR Reviewers

vercel bot commented Aug 6, 2024 • edited Loading

Justkant left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Justkant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RamyEB commented Aug 6, 2024 •

edited by jira bot

Loading

vercel bot commented Aug 6, 2024 •

edited

Loading

Justkant left a comment •

edited

Loading