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.

Sign up for GitHub

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

Jump to bottom

some modals broken after vue 5.0 update (and after vue 5.0 merge in the viewer app) #2203

Closed
5 tasks
szaimen opened this issue Feb 24, 2022 · 4 comments · Fixed by #2312
Closed
5 tasks

some modals broken after vue 5.0 update (and after vue 5.0 merge in the viewer app) #2203

szaimen opened this issue Feb 24, 2022 · 4 comments · Fixed by #2312
Labels
1. to develop bug: regression bug Something isn't working
Milestone
Nextcloud 24

Comments

@szaimen
Copy link
Contributor

szaimen commented Feb 24, 2022 •
edited
Loading

need to verify that all modals work on desktop and mobile after vue 5.0 updates. If not need to make it work.

  • Helpmodal
  • PublicFilesEditor
  • EditorWrapper?
  • ViewerComponent?
  • else?
For easy testing 
docker run -it \
-e VIEWER_BRANCH=master \
-e TEXT_BRANCH=master \
-p 127.0.0.1:8443:443 \
--name nextcloud-easy-test \
ghcr.io/szaimen/nextcloud-easy-test:latest

Feel free to ping me if you should need help on fixing this!
@szaimen szaimen added the bug Something isn't working label Feb 24, 2022
@szaimen szaimen added this to the Nextcloud 24 milestone Feb 24, 2022
@juliushaertl
Copy link
Member

Is there any specific issues you see in regards to the modal? From my quick tests this works fine on master of viewer/server/text.

@szaimen
Copy link
Contributor Author

szaimen commented Mar 1, 2022 •
edited
Loading

Did you test mobile too?

And e.g. opening the sidebar on smaller widths?

@szaimen
Copy link
Contributor Author

szaimen commented Mar 4, 2022

Some examples:

1 2 3
image image image

@juliushaertl
Copy link
Member

juliushaertl commented Mar 4, 2022 •
edited
Loading

For the close button this requires a new release of @nextcloud/vue (see nextcloud-libraries/nextcloud-vue#252)

There is another issue which seems to be related to the viewer where the more button and the text are white if the light theme is used, but that is already under investigation.

And thanks for the screenshot, it seems indeed that there is something wrong with the modal size on mobile

@szaimen szaimen mentioned this issue Mar 24, 2022
Closed
18 tasks
@szaimen szaimen mentioned this issue Apr 16, 2022
Closed
mejo- added a commit that referenced this issue Apr 19, 2022
@mejo-
Fix help modal style with @nextcloud/vue >= 5.0.0
0e6b85a 
Signed-off-by: Jonas <[email protected]>

Fixes: #2203
mejo- added a commit that referenced this issue Apr 19, 2022
@mejo-
Fix height of editor-wrapper in viewer modal on mobile
65e24be 
On mobile, `#modal-container` already has `top: 50px`, so no need to add
another `top: 50px` in `#editor-container` in that case.

Also, remove `top` property from `#editor-container` in EditorWrapper
and RichWorkspace altogether. The correct place to set `top` to fit with
the header bar from a modal is ViewerComponent.

Fixes: #2203
Fixes: #2295

Signed-off-by: Jonas <[email protected]>
@mejo- mejo- mentioned this issue Apr 19, 2022
Merged
mejo- added a commit that referenced this issue Apr 19, 2022
@mejo-
Fix help modal style with @nextcloud/vue >= 5.0.0
7ba1072 
Signed-off-by: Jonas <[email protected]>

Fixes: #2203
mejo- added a commit that referenced this issue Apr 19, 2022
@mejo-
Fix height of editor-wrapper in viewer modal on mobile
e6369f1 
On mobile, `#modal-container` already has `top: 50px`, so no need to add
another `top: 50px` in `#editor-container` in that case.

Also, remove `top` property from `#editor-container` in EditorWrapper
and RichWorkspace altogether. The correct place to set `top` to fit with
the header bar from a modal is ViewerComponent.

Fixes: #2203
Fixes: #2295

Signed-off-by: Jonas <[email protected]>
mejo- added a commit that referenced this issue Apr 19, 2022
@mejo-
Fix help modal style with @nextcloud/vue >= 5.0.0
3feacb9 
Signed-off-by: Jonas <[email protected]>

Fixes: #2203
mejo- added a commit that referenced this issue Apr 19, 2022
@mejo-
Fix height of editor-wrapper in viewer modal on mobile
ce2388d 
On mobile, `#modal-container` already has `top: 50px`, so no need to add
another `top: 50px` in `#editor-container` in that case.

Also, remove `top` property from `#editor-container` in EditorWrapper
and RichWorkspace altogether. The correct place to set `top` to fit with
the header bar from a modal is ViewerComponent.

Fixes: #2203
Fixes: #2295

Signed-off-by: Jonas <[email protected]>
mejo- added a commit that referenced this issue Apr 19, 2022
@mejo-
Fix height of editor-wrapper in viewer modal on mobile
cbc0ec1 
On mobile, `#modal-container` already has `top: 50px`, so no need to add
another `top: 50px` in `#editor-container` in that case.

Also, remove `top` property from `#editor-container` in EditorWrapper
and RichWorkspace altogether. The correct place to set `top` to fit with
the header bar from a modal is ViewerComponent.

Fixes: #2203
Fixes: #2295

Signed-off-by: Jonas <[email protected]>
backportbot-nextcloud bot pushed a commit that referenced this issue Apr 19, 2022
@mejo- @backportbot
Fix help modal style with @nextcloud/vue >= 5.0.0
13eeaf3 
Signed-off-by: Jonas <[email protected]>

Fixes: #2203
backportbot-nextcloud bot pushed a commit that referenced this issue Apr 19, 2022
@mejo- @backportbot
Fix height of editor-wrapper in viewer modal on mobile
08c9a61 
On mobile, `#modal-container` already has `top: 50px`, so no need to add
another `top: 50px` in `#editor-container` in that case.

Also, remove `top` property from `#editor-container` in EditorWrapper
and RichWorkspace altogether. The correct place to set `top` to fit with
the header bar from a modal is ViewerComponent.

Fixes: #2203
Fixes: #2295

Signed-off-by: Jonas <[email protected]>
nextcloud-command pushed a commit that referenced this issue Apr 19, 2022
@mejo- @nextcloud-command
Fix height of editor-wrapper in viewer modal on mobile
48584b2 
On mobile, `#modal-container` already has `top: 50px`, so no need to add
another `top: 50px` in `#editor-container` in that case.

Also, remove `top` property from `#editor-container` in EditorWrapper
and RichWorkspace altogether. The correct place to set `top` to fit with
the header bar from a modal is ViewerComponent.

Fixes: #2203
Fixes: #2295

Signed-off-by: Jonas <[email protected]>
Signed-off-by: nextcloud-command <[email protected]>
nextcloud-command pushed a commit that referenced this issue Apr 19, 2022
@mejo- @nextcloud-command
Fix height of editor-wrapper in viewer modal on mobile
3d46058 
On mobile, `#modal-container` already has `top: 50px`, so no need to add
another `top: 50px` in `#editor-container` in that case.

Also, remove `top` property from `#editor-container` in EditorWrapper
and RichWorkspace altogether. The correct place to set `top` to fit with
the header bar from a modal is ViewerComponent.

Fixes: #2203
Fixes: #2295

Signed-off-by: Jonas <[email protected]>
Signed-off-by: nextcloud-command <[email protected]>
mejo- added a commit that referenced this issue Apr 19, 2022
@mejo-
Fix help modal style with @nextcloud/vue >= 5.0.0
39f5eb5 
Signed-off-by: Jonas <[email protected]>

Fixes: #2203
mejo- added a commit that referenced this issue Apr 19, 2022
@mejo-
Fix height of editor-wrapper in viewer modal on mobile
7d1f43b 
On mobile, `#modal-container` already has `top: 50px`, so no need to add
another `top: 50px` in `#editor-container` in that case.

Also, remove `top` property from `#editor-container` in EditorWrapper
and RichWorkspace altogether. The correct place to set `top` to fit with
the header bar from a modal is ViewerComponent.

Fixes: #2203
Fixes: #2295

Signed-off-by: Jonas <[email protected]>
Signed-off-by: nextcloud-command <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
1. to develop bug: regression bug Something isn't working
Projects
None yet
Milestone
Nextcloud 24
Development

Successfully merging a pull request may close this issue.

Fix modal heights nextcloud/text
2 participants
@juliushaertl @szaimen