feat: Add toggle for editor line length per user

[azri-cs]

📝 Summary

Fixes #4024

After some direct alignment with designers, we basically want to offer a per user setting to allow users changing between a full line length and a compact line length display mode.

🖼️ Screenshots

🏚️ Before	🏡 After

B | A

🏁 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

[codecov]

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

[max-nextcloud]

Hi @azri-cs 👋

Looks like this is your first PR to text. 🎉

❤️ Thanks a lot for contributing! I hope we can walk you through finalizing this.
I assume you are addressing #4024 - so I'll add that to the PR description.

First of all, let's check in with @nextcloud/designers to hear if the ui matches their expectations.

In the meantime we can try and address the failing tests:

DCO

The DCO check is complaining about your signoff message in the commits like this:

If you use git on the command line you can add a signoff message to the last commit with
git commit --amend --signoff.

Lint PHP-cs

At first sight this looks like it's mostly about indentation. Can you see the error message the workflow printed? It has a detailed diff of what needs to be changed.

Psalm

This also looks related:

I hope these are helpful pointers to address these. If you get stuck feel free to ask. I will also take a first look at the code to see if I find something to comment upon.

[max-nextcloud]

Code wise this looks good.

I noticed that you added a string config setting - but I think a boolean would do. If I read #4024 correctly it's either full width or default line length.

[max-nextcloud]

The whitespace of this file somehow changed so that your commit seems to change every line inside the file.
This would be confusing when looking at the history for a given line.

Please try to leave the whitespace untouched. In the server repo there is a .editorconfig file that will tell most editors how we like our indentation.
This may not have any effect for two reasons:

1. You may need an editorconfig extensions / configuration setting to make sure your editor reads .editorconfig files.
2. The .editorconfig file needs ot be in a parent directory of the text repository. Maybe your text repo is not located inside your server repo. I also have that situation. text lives in code/nextcloud/text on my machine. So I copied the .editorconfig to code/nextcloud where it will also affect text.

[azri-cs]

Hi Max,

Thank you for pointing that out. I have pushed the fixes for sign-off issue, whitespaces and backend test failure.

[azri-cs]

Code wise this looks good.

I noticed that you added a string config setting - but I think a boolean would do. If I read #4024 correctly it's either full width or default line length.

Upon reading back the requirements, yes I agree boolean is better since it's a checkbox and there are only 1 option and 1 default. My latest push have implemented this.

[max-nextcloud]

Dear @azri-cs Thanks for changing the approach and using a boolean. That looks good to me.

The linters are still not quite happy. They mostly disagree about the use of whitespace here and there.
For the javascript side of things you can run the linter locally with:
npm run lint
It looks like all the errors it reports can be fixed automatically. You can do this by running
npx eslint --fix --ext .js,.vue src

For php i'm less familiar but my understanding is that you will need to run composer install first to get all the composer dependencies and then you can run what the CI job also runs:
composer run cs:check for checking and
composer run cs:fix for fixing.

[azri-cs]

Dear @azri-cs Thanks for changing the approach and using a boolean. That looks good to me.

The linters are still not quite happy. They mostly disagree about the use of whitespace here and there. For the javascript side of things you can run the linter locally with: npm run lint It looks like all the errors it reports can be fixed automatically. You can do this by running npx eslint --fix --ext .js,.vue src

For php i'm less familiar but my understanding is that you will need to run composer install first to get all the composer dependencies and then you can run what the CI job also runs: composer run cs:check for checking and composer run cs:fix for fixing.

Hi @max-nextcloud

Sorry for the inconvenience caused. I've followed your advice to use .editorconfig from the server repo, and then I run ctrl+alt+l to format each files I previously committed. Not sure why the whitespaces issue still exists despite of this. I've fix the whitespaces issue on my latest push.

[max-nextcloud]

Hey @azri-cs
Thanks for following up on this so swiftly. I will ping the design team again for review. Code wise i think this is ready to be merged.
There's a signature missing on one of the commits - but I think it would make sense to squash all of the commits into one anyway so the back and forth with the whitespace changes does not show up in history.

Could you squash the commits?
Feedback from the design team might not arrive before Monday next week as it's the weekend for most of them now.

[azri-cs]

Hey @azri-cs Thanks for following up on this so swiftly. I will ping the design team again for review. Code wise i think this is ready to be merged. There's a signature missing on one of the commits - but I think it would make sense to squash all of the commits into one anyway so the back and forth with the whitespace changes does not show up in history.

Could you squash the commits? Feedback from the design team might not arrive before Monday next week as it's the weekend for most of them now.

Hi @max-nextcloud

No worries. I've squashed my commits into one commit. Thanks a lot, I've learnt a lot.

[jancborchardt]

Very nice contribution @azri-cs! :) I only have 1 feedback: I would expect a setting like this to be in the collapsed part of the toolbar along with these other more info/settings like items, as the "people" menu is more focused on collaboration and sharing.
(Also, this setting is disabled by default, right?)

[juliusknorr]

@jancborchardt I'm considering this as a follow up, as I scoped the task for @azri-cs to put it explicitly in that menu in the beginning.

[jancborchardt]

@juliusknorr @azri-cs sure, doing it as a follow-up is fine for me too. :)

My remark does make sense though right, or would you still prefer your originally suggested placement?