Skip to content

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)

@CarlSchwan CarlSchwan force-pushed the limit-max-width-settings branch from d6b37a1 to 8d05f6e Compare September 8, 2022 09:01
}
&--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 <[email protected]>
@CarlSchwan CarlSchwan force-pushed the limit-max-width-settings branch from 8d05f6e to 62ca1e3 Compare September 8, 2022 09:23
@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.

5 participants