[$500] Feature Request: Add full screen expand button when editing a messge

kavimuru commented 1 year ago

Held on https://github.com/Expensify/App/issues/16078 If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Problem:

When there's multiple lines being edited, there is a "full-screen expand" button in the upper-left of the compose window, but not when editing an existin

Solution:

Add the full-screen expand button to the upper-left when editing an existing comment

Context/Examples/Screenshots/Notes:

Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/222207842-4cb95747-e11d-4ab7-a3c9-6a3f85cdedd9.mp4

Expensify/Expensify Issue URL: Issue reported by: @quinthar Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1677476774053969

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01e878e8c8ed91741e
Upwork Job ID: 1632674482135707648
Last Price Increase: 2024-11-19

Issue Owner

Current Issue Owner: @s77rt

MelvinBot commented 1 year ago

Triggered auto assignment to @sonialiap (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[ ] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[ ] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

sonialiap commented 1 year ago

On web, when editing a message the edit composer opens to show the entire message if it's <=16 lines. The editor does not currently expand beyond 16 lines.

sonialiap commented 1 year ago

I can't think of a reason for why we wouldn't want to allow expanding the editor to full screen 👍

MelvinBot commented 1 year ago

Triggered auto assignment to @cristipaval (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

cristipaval commented 1 year ago

This could be external.

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01e878e8c8ed91741e

MelvinBot commented 1 year ago

Current assignee @sonialiap is eligible for the External assigner, not assigning anyone new.

MelvinBot commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @s77rt (External)

MelvinBot commented 1 year ago

Current assignee @cristipaval is eligible for the External assigner, not assigning anyone new.

tienifr commented 1 year ago

Since this is similar to the existing Composer and is quite straight-forward, I can take it if it's on volunteer-basis.

We can add this component https://github.com/Expensify/App/blob/f2bded26692e7b462c758241860e351e524af730/src/pages/home/report/ReportActionCompose.js#L519 into the src/pages/home/report/ReportActionItemMessageEdit.js

s77rt commented 1 year ago

@tienifr I believe this still requires proposals to follow the template.

tienifr commented 1 year ago

Edited by proposal-police: This proposal was edited at 2024-10-03 09:28:01 UTC.

@s77rt Sure, here's my proposal.

Proposal

Please re-state the problem that we are trying to solve in this issue.

When there's multiple lines being edited, there is a "full-screen expand" button in the upper-left of the compose window, but not when editing an existing.

What is the root cause of that problem?

This is because we don't have that feature yet when editing the message.

What changes do you think we should make in order to solve the problem?

We can add this component https://github.com/Expensify/App/blob/f2bded26692e7b462c758241860e351e524af730/src/pages/home/report/ReportActionCompose.js#L519 that is already in ReportActionComposer into the src/pages/home/report/ReportActionItemMessageEdit.js. Specifically above this component: https://github.com/Expensify/App/blob/aafc79f3587ea0b8209468021761f92f556184fe/src/pages/home/report/ReportActionItemMessageEdit.js#L232

The logic should be similar as well:

There'll be a new setIsEditComposerFullSize to indicate which edit composer is currently full size (similar to the setIsComposerFullSize)
The full size state needs to be passed into the ReportActionItemMessageEdit by using useOnyx

    const [isComposerFullSize] = useOnyx(`reportActionsDrafts_ ${reportID}`, {
        selector: (actionDraft) => {
            return actionDraft?.[action.reportActionID].isFullSize;
        },
        initialValue: false
    });

We'll apply full size style to the composer using the full size state here

style={[styles.chatItemMessage, styles.flexRow, isComposerFullSize && {
                    height: windowHeight - HEADER_HEIGHT - (shouldShowComposeInput && mainComposerHeight) - 16 // padding
                }]}

We can store the main composer height in Onyx and get its value a.k.a mainComposerHeight in ReportActionItemMessageEdit

Enable isFullSize when users press Expand button
Disable isFullSize when users press Collapse button or dismiss edit mode
Hide main composer if isFullSize (from reportActionsDrafts_) is true and the edit composer is focused (from ONYXKEYS.SHOULD_SHOW_COMPOSE_INPUT)

What alternative solutions did you explore? (Optional)

NA

Result

This is a very draft screenshot of me implementing the change above, the exact design will depend on the design team.

https://github.com/user-attachments/assets/f0c245e5-9d19-4766-a04e-b8042d4d28e0

s77rt commented 1 year ago

Thanks for the proposal @tienifr. One thing I'm concerned about is how will you make the composer take full height being a FlatList item.

tienifr commented 1 year ago

@s77rt we can do that by making the height of the ReportMessageItem equal to the computed height of the Report Dropzone https://github.com/Expensify/App/blob/9591146685c52e0b00bc9fdeef4aa5ee61bafe61/src/pages/home/ReportScreen.js#L253, if we want to keep the main composer visible then we'll minus the height of it from the Report Dropzone height.

Here's the demo:

s77rt commented 1 year ago

@tienifr Thanks for the quick reply. I still need to get aligned with you on some points:

Will we still be able to scroll the messages? I think we should prevent that
Can we adjust the height without relaying on another component. Dropzone have nothing to do with message edit here.

tienifr commented 1 year ago

@s77rt thanks, your mentioned points can be addressed by: If there's a current full-size report action component, we'll return only 1 ReportActionItem and do not return the full ReportActionsList, the layers of containers of ReportActionItem will be set to have flex: 1 so it will occupy the full screen height, similar to the current behavior of ReportActionCompose when it has full screen height

Nayan120603 commented 1 year ago

i read your problem of expanding button feature. here i wrote some codes for it. may be it will helpful..

here it is https://1drv.ms/w/s!Amsi9ANVEhq5ghTx-bZVbg7Y95Gt?e=XBuWZf

upwork link : https://www.upwork.com/freelancers/~0107791b59dea64168

MelvinBot commented 1 year ago

📣 @Nayan120603! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Screen Shot 2022-11-16 at 4 42 54 PM

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

s77rt commented 1 year ago

@tienifr Thanks for the clarification. I think your proposal is getting a little bit complex, let me get a confirmation for the expected behaviour first.

@cristipaval @sonialiap Can you confirm if the expected behaviour is to show the edit composer full height and prevent scrolling to other messages while in this state? Or the rest of message should still be visible and reachable.

s77rt commented 1 year ago

@Nayan120603 Thanks for your interest. Please read the contributing guide and follow the proposal template.

cristipaval commented 1 year ago

@cristipaval @sonialiap Can you confirm if the expected behaviour is to show the edit composer full height and prevent scrolling to other messages while in this state? Or the rest of message should still be visible and reachable.

I think we should be consistent and make it behave the same as when creating a new message. If you read the Slack conversation, the reporter was expecting to see the same behaviour.

s77rt commented 1 year ago

@cristipaval Thanks! I have asked on that slack thread.

s77rt commented 1 year ago

@tienifr Can you update your proposal given that we would keep the report actions items untouched (they won't be visible since the report action being edited should take full height) but we should prevent flat list from scrolling.

How would you make the report action take full height
How will you prevent scrolling of the flatlist

tienifr commented 1 year ago

@s77rt what do you think about this? https://github.com/Expensify/App/issues/15596#issuecomment-1455980766

It will work for both the points you mentioned. Having a not-scrollable flat list with only 1 item visible as full-height is the same as showing only that 1 item.

s77rt commented 1 year ago

@tienifr The additional logic of rendering only one item although it may work it seems unnecessary, it would be better to just work on top of what we have, set the height of the report action to take full height and prevent scrolling.

tienifr commented 1 year ago

@s77rt sure, if we want to keep the report actions list then we can do this:

How would you make the report action take full height

If an edit composer currently has full height state, we should make the FlatList and all levels of containers of the Edit composer to have flex: 1/height: 100%. This will make sure the Action Item that has the edit composer will stretch to have full height.

How will you prevent scrolling of the flatlist

We need to apply scrollEnabled: false to the Flatlist (which will work for native) and overflow: hidden (which will work for web, we might consider adding an upstream fix to react-native-web since the scrollEnabled: false property is not working there)

It will show like below after the above changes:

s77rt commented 1 year ago

@tienifr Thanks for the quick update, can you elaborate where to put overflow: hidden? I can still scroll the flat list.

tienifr commented 1 year ago

@s77rt sure, it should be added to the contentContainerStyle here https://github.com/Expensify/App/blob/2c35e7abfeab34b3e1ee03207d93ef7984f80a52/src/pages/home/report/ReportActionsList.js#L183 Make sure to have both these properties: {overflow: 'hidden', flex: 1}, then it won't be scrollable

s77rt commented 1 year ago

@tienifr Thanks once again for the clarification. I think the provided info are enough to start working on a PR, other details will be discussed there.

:ribbon: :eyes: :ribbon: C+ reviewed

cc @cristipaval

tienifr commented 1 year ago

thanks @s77rt for the review, since it's a feature request should we have design's confirmation on the UI (things like location of the expand button, how full-page editing would look) before moving forward?

We have 2 UI cases here:

The expand button on the MessageEdit
The full-screen edit after clicking on the expand button

s77rt commented 1 year ago

@tienifr The layout should be the same as that of the main composer, the expand button should appear on the left.

cristipaval commented 1 year ago

Thanks @s77rt ! Looks good to me. @tienifr , go ahead with your solution 🚀

MelvinBot commented 1 year ago

📣 @tienifr You have been assigned to this job by @cristipaval! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

cristipaval commented 1 year ago

So sorry, @tienifr! I forgot to assign you.

tienifr commented 1 year ago

@s77rt @cristipaval

I am encountering this on the main branch, and I hope I could get more eyes on this. The newest code on the main branch, merged from this PR has removed the full screen size for the main composer. In order for the new emoji suggestion function to work, when the user expand the main composer, it still leaves a space with report actions being displayed and scrollable.

I think this contradicts to what we have earlier agreed upon the current feature request, so I think we need to re-evaluate on what need to be done.

Do we still need to make the edit composer full screen, hide other report action items and disable scrolling? Or should we make it similar to the newest main composer?
Do we need to make the emoji suggestion available with the edit composer?

For the sake of consistency, I think we should make it as similar as the main composer as possible. That means we would still need to display all the other messages, do not disable scrolling, and make the emoji suggestion available to the edit composer.

What do you think?

s77rt commented 1 year ago

I'm not sure, seems like a regression. cc @roryabraham
For consistency I think we will but not in this issue. I assume it will be handled in another separated issue

@cristipaval thoughts?

tienifr commented 1 year ago

@s77rt I think that's not a regression. The style is intentionally modified so that the new emoji suggestions are visible. The suggestions would not be displayed if the composer height is 100%.

Either way, let's wait for @cristipaval input.

cristipaval commented 1 year ago

Slack discussion here

MelvinBot commented 1 year ago

@s77rt @sonialiap @cristipaval @tienifr this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

s77rt commented 1 year ago

Still waiting for a clarification on the expected behaviour. Slack discussion.

s77rt commented 1 year ago

@tienifr The changes are intentional, so just add the expand button and verify it has same behavior as on Main Composer. Looks like that the hard part has been cut down :tada:

sonialiap commented 1 year ago

Just FYI, I'll be OOO Mar 17-25

cristipaval commented 1 year ago

@s77rt @tienifr According to the last replies in this Slack convo, we might revert the changes which limited the composer height and go full space again. I think we should put this one on old until the team implements the changes to support both full screen composer and emoji suggestions on top of it.

s77rt commented 1 year ago

@cristipaval Makes sense. Let's hold this.

cristipaval commented 1 year ago

This one is held on https://github.com/Expensify/App/issues/16078

s77rt commented 1 year ago

On hold...

s77rt commented 1 year ago

Please @MelvinBot this is really on hold

s77rt commented 1 year ago

Still on hold

roryabraham commented 1 year ago

Making this a weekly while it's on HOLD

Expensify / App