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

Limit max width of the settings #3192

Merged
merged 1 commit into from
Sep 8, 2022
Merged

Conversation

CarlSchwan
Copy link
Contributor

This option only apply to the title and description by default and is opt-in for the content.

@CarlSchwan CarlSchwan added the 3. to review Waiting for reviews label Sep 7, 2022
@CarlSchwan CarlSchwan self-assigned this Sep 7, 2022
@jancborchardt
Copy link
Contributor

Wouldn't it work better if we just limit the width of h2, h3, etc and p elements by default, not making it options?

That would limit the width of the text-based parts, keep layouts of e.g. Logging, and also ensure devs don't forget to set it. So we don't need an option devs need to set.

@CarlSchwan
Copy link
Contributor Author

There is probably more elements we want to limit the width than just p, h1, h2, ... like the notecard components, input fields, and more

The idea of @skjnldsv to make it on by default, is probably a good one since nc7.0 is a breaking release anyway. We just need to document somewhere that if someone is using NcSettingSection to check if they might want to disable this behavior (e.g. activity app)

@@ -117,12 +130,17 @@ export default {
border-bottom: 1px solid var(--color-border);
}

&--limit-width > * {
max-width: 900px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
max-width: 900px;
max-width: $maxWidth;

This option only apply to the title and description by default and is
opt-in for the content.

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@jancborchardt
Copy link
Contributor

Ok, sounds good. The admin Logging app is definitely one which needs an exception, but I think it's written in React anyway @icewind1991?

@CarlSchwan
Copy link
Contributor Author

Ok, sounds good. The admin Logging app is definitely one which needs an exception, but I think it's written in React anyway @icewind1991?

it is, same with the groupfolder app. At some point we need to move that to vue but this can probably wait :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants