-
Notifications
You must be signed in to change notification settings - Fork 109
fix(readonly-bar): Clean up read-only menu bar to behave consistently with normal menu bar #7560
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
Conversation
max-nextcloud
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for looking into this. 💙
I just had a brief look at the code and so far it looks good. There's just one aspect that needs to be addressed.
I'll try this out later.
| <ReadonlyBar | ||
| v-if="readOnly || (openReadOnlyEnabled && !editMode)" | ||
| class="text-editor--readonly-bar"> | ||
| <slot name="readonlyBar"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This slot is used by the collectives app to alter the content of the read only bar. I think we need to keep it so that collectives can continue doing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, sorry about that! I readded the slot. However, it's now a direct child of the MainContainer component, which is the same setup as here:
text/src/components/Editor/MarkdownContentEditor.vue
Lines 11 to 19 in f405547
| <MainContainer> | |
| <template v-if="showMenuBar"> | |
| <MenuBar v-if="!readOnly" :autohide="false" /> | |
| <slot v-else name="readonlyBar"> | |
| <ReadonlyBar /> | |
| </slot> | |
| </template> | |
| <ContentContainer /> | |
| </MainContainer> |
Do you think that could be an issue? Personally, I feel it’s better to let the ReadonlyBar handle its own CSS styling.
20ee01a to
07b3809
Compare
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
… with normal menu bar Signed-off-by: Peter Birrer <[email protected]>
07b3809 to
152e1cb
Compare
mejo-
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for tackling this @pbirrer 🙏
I tested it both in standalone Text and Collectives and it looks much cleaner now.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7560 +/- ##
==========================================
+ Coverage 52.93% 59.77% +6.83%
==========================================
Files 503 502 -1
Lines 43829 38966 -4863
Branches 1129 1128 -1
==========================================
+ Hits 23203 23291 +88
+ Misses 20517 15567 -4950
+ Partials 109 108 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Jonas <[email protected]>
Signed-off-by: Jonas <[email protected]>
|
/backport to stable32 |
|
/backport to stable31 |
📝 Summary
This pull request addresses several issues related to read-only mode:
🏁 Checklist
npm run lint/npm run stylelint/composer run cs:check)