Skip to content

Comments

fix: add close button when open readonly files on mobile app#7624

Merged
mejo- merged 1 commit intomainfrom
add-close-button-on-readonly
Sep 22, 2025
Merged

fix: add close button when open readonly files on mobile app#7624
mejo- merged 1 commit intomainfrom
add-close-button-on-readonly

Conversation

@luka-nextcloud
Copy link
Contributor

@luka-nextcloud luka-nextcloud commented Sep 9, 2025 •
edited
Loading

📝 Summary

Add close button when open read-only files on mobile app

🖼️ Screenshots

🏚️ Before 🏡 After
image image

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

@luka-nextcloud luka-nextcloud requested a review from a team September 9, 2025 16:33
@codecov
Copy link

codecov bot commented Sep 9, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.54%. Comparing base (ea23129) to head (9526394).
⚠️ Report is 22 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #7624   +/-   ##
=======================================
  Coverage   59.54%   59.54%           
=======================================
  Files         310      310           
  Lines       38748    38754    +6     
  Branches      937      937           
=======================================
+ Hits        23072    23078    +6     
  Misses      15568    15568           
  Partials      108      108

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mejo-
mejo- reviewed Sep 9, 2025
View reviewed changes
Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this! I didn't test yet, just reviewed the code changes.

@luka-nextcloud luka-nextcloud force-pushed the add-close-button-on-readonly branch from 021522c to 8b770e2 Compare September 11, 2025 09:37
@luka-nextcloud luka-nextcloud added bug Something isn't working 3. to review labels Sep 11, 2025
@luka-nextcloud luka-nextcloud moved this to 👀 In review in 📝 Productivity team Sep 11, 2025
@luka-nextcloud luka-nextcloud force-pushed the add-close-button-on-readonly branch from 8b770e2 to 00c720c Compare September 16, 2025 11:34
mejo-
mejo- requested changes Sep 16, 2025
View reviewed changes
Copy link
Member

@mejo- mejo- left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I just now took a deeper look. I think there's no need to add the button to the Editor.vue component manually. Instead, you should make sure that the #header template from DirectEditing.vue is added to the <ReadOnlyBar> element in Editor.vue just like it's done for <MenuBar>.

While testing this, I realized that the ReadOnlyBar doesn't display well on mobile (at the bottom), it's cut off and the border line is on the wrong side. If you could take a look into improving this and harmonizing it with how MenuBar is displayed on mobile, that would be awesome. If not we can also do it in a follow-up PR. (already tackled in #7560)

@max-nextcloud
Copy link
Collaborator

max-nextcloud commented Sep 17, 2025

#7560 seems related.

@mejo-
Copy link
Member

mejo- commented Sep 17, 2025

#7560 seems related.

Ah wow, I missed that. It seems to address my second comment from above 🙏 Will take a look.

@mejo-
Copy link
Member

mejo- commented Sep 17, 2025

While testing this, I realized that the ReadOnlyBar doesn't display well on mobile (at the bottom), it's cut off and the border line is on the wrong side. If you could take a look into improving this and harmonizing it with how MenuBar is displayed on mobile, that would be awesome. If not we can also do it in a follow-up PR.

So this is no longer necessary as it got tackled in #7560 already 🙏

@luka-nextcloud luka-nextcloud force-pushed the add-close-button-on-readonly branch from 00c720c to 9526394 Compare September 18, 2025 12:01
@mejo- mejo- merged commit 158fa49 into main Sep 22, 2025
66 checks passed
@mejo- mejo- deleted the add-close-button-on-readonly branch September 22, 2025 09:16
@github-project-automation github-project-automation bot moved this from 👀 In review to ☑️ Done in 📝 Productivity team Sep 22, 2025
@mejo-
Copy link
Member

mejo- commented Sep 22, 2025

/backport to stable32

@backportbot backportbot bot mentioned this pull request Sep 22, 2025
Merged
@mejo-
Copy link
Member

mejo- commented Sep 22, 2025

/backport to stable30

@backportbot backportbot bot mentioned this pull request Sep 22, 2025
Merged
@mejo-
Copy link
Member

mejo- commented Sep 22, 2025

/backport to stable31

@backportbot backportbot bot mentioned this pull request Sep 22, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mejo- mejo- mejo- approved these changes

@max-nextcloud max-nextcloud Awaiting requested review from max-nextcloud max-nextcloud is a code owner

Assignees

No one assigned

Labels

3. to review bug Something isn't working

Projects

📝 Productivity team
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Close button does not show when opening readonly .txt or .md files in iOS and Android Nextcloud app

3 participants

@luka-nextcloud @max-nextcloud @mejo-