LF-4474 [Nice to have] convert create task button to CTA floating button #3540
Conversation
kathyavini
requested review from
antsgar and
Duncan-Brain
and removed request for
a team
| position: fixed; | ||
| bottom: 16px; | ||
| right: 16px; | ||
| z-index: 100; |
There was a problem hiding this comment.
I wonder if we should set a z-index on the floating button component itself since it's supposed to always float over everything else?
There was a problem hiding this comment.
That makes sense to me. Do you think the positioning CSS should even be controlled by a prop on the component if we're using it so often in this corner? 🤔
There was a problem hiding this comment.
Oh yes, that's a really good idea!
| </Layout> | ||
|
|
||
| <div className={styles.buttonWrapper}> | ||
| <FloatingActionButton type={'add'} onClick={handleAddTask} /> |
There was a problem hiding this comment.
I think you brought this up before, but do we want the button to trigger the action directly, or to open the floating menu like we're doing with Animals? It does make sense to just trigger the action if there's only one option, but we could double check with Loïc (if we haven't) and if this is the way make it consistent in Animals as well.
There was a problem hiding this comment.
Thanks for bring it up! As discussed with Loïc today, I'll make them both one-click (no menu).
|
Looks like the position is maybe different on animals or something? Only other thing is it would be good to keep the "Add a task" translated text for screen readers and hover or something as discussed in design.
|
|
@Duncan-Brain that's a great idea about the screen readers! I kept the string deletion in one commit just in case it had to be reverted 😉 I'm not sure what you're seeing with the positioning though hmm 🤔 For me if I switch between task and inventory the button doesn't move at all (which should be as expected; I took the positioning CSS from FloatingMenuButton). Can you check you're definitely on the base branch, updated version? Edit: actually could it be a scrollbar situation? Could scrollbars being present shift what is considered the edge of the screen?? I guess both of your screens have scrollbars though. Stumped! |
|
@kathyavini I am only seeing the FAB hug the scrollbar on tablet like sizes where the thick scrollbar is present. I also tried choosing tablet devices like ipad Pro and saw they had thick scrollbars vs ipad Mini and cell phones which have thin scrollbars. Core difference seems to be FAB in inventory is part of table vs app wrapper. It looks like FAB should be moved up one in inventory the hierarchy to match / avoid scrollbar issues. EDIT: Meant to say it is not a blocker for me for position since task seems correct |
This reverts commit 2aa86ec.
…l Inventory as well
|
@Duncan-Brain interesting, I can't even reproduce at tablet widths. I just updated the component in inventory as we are no longer using the menu. Does it make any difference? |
kathyavini
changed the title
Description
This PR removes the "+ Create a task" text link on TaskPage and moves that functionality to a FloatingActionButton. There was also a check for locations and a creation location modal tucked into the
<TaskCount />component that were lifted up to the parent<TaskPage />. This was by necessity, but I think it's also more logical; I was personally surprised to find that logic co-located with the count display!Jira link: https://lite-farm.atlassian.net/browse/LF-4474
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: