Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Implement Home view tasks section list #10961

Merged
merged 5 commits into from
Dec 4, 2024

Conversation

olerichter00
Copy link
Contributor

@olerichter00 olerichter00 commented Oct 14, 2024

This PR resolves EMI-2096

Description

This PR implements the grouping of multiple tasks on the home view
It also adds the animation to the tasks when expanding

Demo

Android iOS
grouped-tasks-android.mov
grouped-tasks-ios.mov

PR Checklist

  • I have tested my changes on iOS and Android.
  • I hid my changes behind a feature flag, or they don't need one.
  • I have included screenshots or videos, or I have not changed the UI.
  • I have added tests, or my changes don't require any.
  • I added an app state migration, or my changes do not require one.
  • I have documented any follow-up work that this PR will require, or it does not require any.
  • I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

  • I would like at least one of the reviewers to run this PR on the simulator or device.
Changelog updates

Changelog updates

Cross-platform user-facing changes

  • implement task grouping in the Home View - MrSltun

iOS user-facing changes

Android user-facing changes

Dev changes

Need help with something? Have a look at our docs, or get in touch with us.

@ArtsyOpenSource
Copy link
Contributor

ArtsyOpenSource commented Oct 14, 2024

This PR contains the following changes:

  • Cross-platform user-facing changes (Implement Home view tasks section list (behind feature flag) - ole - olerichter00)

Generated by 🚫 dangerJS against 9cdc209

@olerichter00 olerichter00 self-assigned this Oct 14, 2024
@olerichter00 olerichter00 requested a review from MrSltun October 14, 2024 16:05
@MrSltun MrSltun force-pushed the olerichter00/EMI-2103/homview-tasks-section-list branch from a1569e5 to 9fbbfc2 Compare November 21, 2024 10:25
@olerichter00 olerichter00 assigned MrSltun and unassigned olerichter00 Nov 21, 2024
@MrSltun MrSltun force-pushed the olerichter00/EMI-2103/homview-tasks-section-list branch from 9fbbfc2 to 40a1ccb Compare November 26, 2024 09:45
@MrSltun MrSltun force-pushed the olerichter00/EMI-2103/homview-tasks-section-list branch from fbbc2ec to 9cdc209 Compare December 3, 2024 20:03
Comment on lines +107 to +141
const renderItem = useCallback<ListRenderItem<Task>>(
({ item, index }) => {
let scaleX = 1
let translateY = 0
let opacity = 1

if (!showAll && index !== 0) {
scaleX = 1 - index * 0.05
translateY = -83 * index
opacity = 1 - index * 0.15
if (index > 2) {
opacity = 0
}
}

return (
<Flex>
<MotiView
key={item.internalID + index}
transition={{ type: "timing", duration: 500 }}
animate={{ transform: [{ scaleX }, { translateY }], opacity }}
>
<Task
disableSwipeable={displayTaskStack}
onClearTask={() => handleClearTask(item)}
onPress={displayTaskStack ? () => setShowAll((prev) => !prev) : undefined}
ref={swipeableRef}
task={item}
/>
</MotiView>
</Flex>
)
},
[displayTaskStack, handleClearTask, showAll]
)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this is basically a memoized component. I've seen this before in other apps but wonder if there is an established convention for functions called renderSomething that return a JSX.Element vs defining them as a standard functional component with a props signature.

Copy link
Member

Choose a reason for hiding this comment

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

I believe both would work, but having them as a memoized function makes the list rendering more performant than rendering a component without a memoization

In this case, I used it as a function inside the component to be able to access some of the states and values that are in the main component instead of passing it down to another component

Copy link
Contributor

@erikdstock erikdstock Dec 4, 2024

Choose a reason for hiding this comment

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

Oh right, I didn't look closely but assumed it was inside for access to some state. My general preference would just be to name it like a component and give it a props-style object arg, still memoizing it. This way when it's used in code it would appear as <Foo bar={true} baz={42} /> rather than {renderFoo(true, 42)}.

But as I look at this more it is actually being passed to a renderItem prop and so I would probably throw everything I just said out the window.

Copy link
Contributor

Choose a reason for hiding this comment

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

Last q: Shouldn't key be in the wrapping flex component?

Copy link
Member

Choose a reason for hiding this comment

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

Last q: Shouldn't key be in the wrapping flex component?

I believe that's handled by Flatlist's keyExtractor prop, but I used that key for MotiView to identify which item to animate how, but I can try to remove it and see what happens :)

Copy link
Contributor

@erikdstock erikdstock left a comment

Choose a reason for hiding this comment

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

Looking good to me, a few basic comments but nothing blocking.

Comment on lines +96 to +98
if (!task) {
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Necessary?

Copy link
Member

Choose a reason for hiding this comment

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

I guess not 😅 I'll remove it!

const [clearedTasks, setClearedTasks] = useState<string[]>([])
const [showAll, setShowAll] = useState(false)

const filteredTasks = tasks.filter((task) => !clearedTasks.includes(task.internalID))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused here as this seems like the only usage of clearedTasks. Why are these kept in state - does it have to do with the relay store and what happens after you swipe one away?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's correct. I tried updating the tasks list in the relay store but I wasn't successful with it, maybe I was missing something there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My idea here was to immediately hide all swiped-away tasks without having to refetch the list of tasks. I guess this could (in theory 🙃) be possible only with the Relay store and without using the extra state.

If the dismissTask mutation queried for the dismissedAt field and had an optimistic response implemented, this could lead to Relay updating the task in the connection within the Relay store. But I'm not sure if this would work in practice...

Copy link
Member

@MrSltun MrSltun Dec 4, 2024

Choose a reason for hiding this comment

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

Oh Thank you Ole! I can try that with a follow-up PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's also possible to Imperatively update the Relay store, but it's not super straightforward....

@MrSltun
Copy link
Member

MrSltun commented Dec 4, 2024

Merging this to prepare for the QA

@MrSltun MrSltun merged commit 70c2919 into main Dec 4, 2024
7 checks passed
@MrSltun MrSltun deleted the olerichter00/EMI-2103/homview-tasks-section-list branch December 4, 2024 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants