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

fix: use body element if not the fullscreen #11222

Merged
merged 3 commits into from Dec 18, 2023

Conversation

Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Dec 13, 2023

☑️ Resolves

Should be safe to use a body element while you're not in the fullscreen (as tested with ViewerOverlayCallView).
Also as long as we won't register a fullscreen state in sidebar (for Files page, for example), modal will end up in body, so above the Viewer embedded

Warning

Behaviour change: as NcModal component has more reasons against being re-mounted, then pros, hotkey 'F' to toggle Fulscreen mode is now disabled when any modal is open 368e43a

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

🖼️ Window frame 🏞️ Fullscreen
Files Sidebar -
Screenshot from 2023-12-13 14-38-32 Screenshot from 2023-12-13 14-38-55
Screenshot from 2023-12-13 14-38-22 Screenshot from 2023-12-13 14-38-46
Talk main app -
Screenshot from 2023-12-13 14-36-49 Screenshot from 2023-12-13 14-37-17
Screenshot from 2023-12-13 14-36-26 Screenshot from 2023-12-13 14-36-03
Screenshot from 2023-12-13 14-34-50 Screenshot from 2023-12-13 14-35-02

🚧 Tasks

  • Check possible drawbacks?

🏁 Checklist

  • 🌏 Tested with Chrome, Firefox and Safari or should not be risky to browser differences

@Antreesy Antreesy added 3. to review bug feature: frontend 🖌️ "Web UI" client feature: talk-sidebar ⬅️ Sidebar integration of Talk into other apps like sharing and documents labels Dec 13, 2023
@Antreesy Antreesy self-assigned this Dec 13, 2023
@Antreesy Antreesy added this to the 💞 Following Major (29) milestone Dec 13, 2023
Copy link
Contributor

@DorraJaouad DorraJaouad left a comment

Choose a reason for hiding this comment

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

2 things to check :

  • Event propagation where modals are involved ( like @event or v-on )
  • CSS dependency ( using :deep to affect a modal style), as the modal may be out of scope.

Otherwise, should be fine.

@Antreesy Antreesy marked this pull request as draft December 14, 2023 11:55
@Antreesy
Copy link
Contributor Author

  • CSS dependence ( using :deep to affect a modal style), as the modal may be out of scope.

Elements inside #content-vue have inherited box-sizing: border-box.
Others attached to body - depends in each case

@Antreesy
Copy link
Contributor Author

Fixed for NewMessage in ChatView:

It provides container to children components (EmojiPicker, autocomplete, NcActions) when mounted, and won't update anymore.
So we need to listen to fullscreen change and re-mount component accordingly

@Antreesy
Copy link
Contributor Author

Antreesy commented Dec 14, 2023

Found a drawback: if you go to Fullscreen with open modal, it will stay attached to the body element, thus won't be visible. Freshly opened modals works fine, as they get an actual container value before being mounted
Possible solution:

  • attach :key="container" to each modal to re-mount i:eyes: - breaks video stream in MediaSettings (at least)
  • disable hotkey [F] if modal is open - seems good as it' not expected?

@DorraJaouad
Copy link
Contributor

Possible solution: attach :key="container" to each modal

As it is causing re-render only when container changes, sounds like a good solution.

@Antreesy
Copy link
Contributor Author

NcActions works fine, as they're re-rendered and re-mounted each time the container is changed
Looks like this function is triggered each time: https://vuejs.org/api/options-rendering.html#render

https://github.com/nextcloud-libraries/nextcloud-vue/blob/ebbe5c689959fc6470ab79352fd8126d3b98aec2/src/components/NcActions/NcActions.vue#L1215-L1221

@Antreesy Antreesy marked this pull request as ready for review December 15, 2023 10:51
Copy link
Contributor

@DorraJaouad DorraJaouad left a comment

Choose a reason for hiding this comment

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

Nothing is blocking, can't find another drawback.

src/components/TopBar/TopBarMenu.vue Show resolved Hide resolved
Copy link
Contributor

@DorraJaouad DorraJaouad left a comment

Choose a reason for hiding this comment

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

Approved again

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@DorraJaouad
Copy link
Contributor

Fixup squash and rebased

@DorraJaouad
Copy link
Contributor

/backport to stable28

@DorraJaouad
Copy link
Contributor

@nickvergessen backport to stable27 and stable26 ?

@nickvergessen
Copy link
Member

My take would be no 🤔

@nickvergessen
Copy link
Member

/backport to stable27

@nickvergessen nickvergessen merged commit f0af070 into main Dec 18, 2023
36 checks passed
@nickvergessen nickvergessen deleted the fix/8709/set-modal-targeting branch December 18, 2023 14:01
@backportbot-nextcloud
Copy link

The backport to stable27 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable27
git pull origin stable27

# Create the new backport branch
git checkout -b fix/foo-stable27

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable27

Error: Unknown error

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

@nickvergessen
Copy link
Member

@DorraJaouad ^ can you take care of the backport while Maksim is out?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug feature: frontend 🖌️ "Web UI" client feature: talk-sidebar ⬅️ Sidebar integration of Talk into other apps like sharing and documents
Projects
None yet
4 participants