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

Remove bottom margin for better warning blocks display #5310

Merged
merged 1 commit into from
Mar 25, 2024

Conversation

Jerome-Herbinet
Copy link
Member

@Jerome-Herbinet Jerome-Herbinet commented Jan 24, 2024

This CSS change removes the extra bottom padding (which is in fact a paragraph bottom margin).
It makes text more compact, which is IMO relevant because warning blocks aim to be compact.

🏚️ Before

Capture d’écran du 2024-01-24 09-21-10

🏑 After

Capture d’écran du 2024-01-24 09-21-17

Fixes: nextcloud/collectives#1175

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits

@Jerome-Herbinet Jerome-Herbinet added design Experience, interaction, interface, … 3. to review labels Jan 24, 2024
Copy link
Member

@juliusknorr juliusknorr 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 the contribution. Small comment inside as I think the rule is applying to widely.

src/css/prosemirror.scss Outdated Show resolved Hide resolved
@Jerome-Herbinet Jerome-Herbinet force-pushed the Jerome-Herbinet-warning-block-remove-text-bottom-margin branch from 7315944 to 411a0e0 Compare January 25, 2024 09:12
@Jerome-Herbinet
Copy link
Member Author

@juliushaertl please check my new commit (old and new commit have been squashed)

@Jerome-Herbinet
Copy link
Member Author

Testing by your side is welcome

@@ -103,6 +103,10 @@ div.ProseMirror {
line-height: 150%;
}

.callout p .paragraph-content {
Copy link
Member

Choose a reason for hiding this comment

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

We should still only do this for the last p element within a callout and please move this style to the actual callout vue component linked earlier

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm sorry @juliushaertl, but what is the actual linked callout vue component you're talking about ?

Copy link
Member

@mejo- mejo- Jan 26, 2024

Choose a reason for hiding this comment

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

@Jerome-Herbinet https://github.com/nextcloud/text/blob/main/src/nodes/CodeBlockView.vue is the component @juliushaertl is talking about.

See the <style> block at the end of this file. There you can overwrite the behaviour for p .paragraph-content, but as @juliushaertl said it should only be done if it's the last p element.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your answer @mejo- but there is no p .paragraph-content css property to overwrite in https://github.com/nextcloud/text/blob/main/src/nodes/CodeBlockView.vue <style> block πŸ€”

@Jerome-Herbinet Jerome-Herbinet force-pushed the Jerome-Herbinet-warning-block-remove-text-bottom-margin branch from 411a0e0 to a7ae981 Compare January 26, 2024 12:37
@juliusknorr juliusknorr self-requested a review February 15, 2024 21:17
@mejo- mejo- force-pushed the Jerome-Herbinet-warning-block-remove-text-bottom-margin branch from a7ae981 to dd1e1d4 Compare March 25, 2024 14:08
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.

Turned out Callout.vue already had a rule to remove margin-bottom for the last child, but it was broken. I took the liberty to push a fix for this.

Fixes: nextcloud/collectives#1175

Signed-off-by: JΓ©rΓ΄me Herbinet <33763786+Jerome-Herbinet@users.noreply.github.com>
Signed-off-by: Jonas <jonas@freesources.org>
@mejo- mejo- force-pushed the Jerome-Herbinet-warning-block-remove-text-bottom-margin branch from dd1e1d4 to 9ca90fc Compare March 25, 2024 14:10
@mejo-
Copy link
Member

mejo- commented Mar 25, 2024

/backport to stable28

@mejo- mejo- merged commit 3c9e987 into main Mar 25, 2024
56 of 58 checks passed
@mejo- mejo- deleted the Jerome-Herbinet-warning-block-remove-text-bottom-margin branch March 25, 2024 14:34
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review design Experience, interaction, interface, … feedback-requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vertically center text inside callouts
3 participants