bulma icon indicating copy to clipboard operation
bulma copied to clipboard

Fix usage of ltr/rtl padding for notifications (Fixes #3025)

Open shreve opened this issue 3 years ago • 2 comments

This is a bugfix. Addresses #3025

Proposed solution

Unify the usage of ltr and rtl padding values behind one variable, $notification-padding.

Currently, $notification-padding is set, but isn't used at all. The real padding is written using +ltr / +rtl mixins, and using the specific values $notification-padding-ltr and $notification-padding-rtl.

This obscures what we really want to do: Offer a configurable value for padding, and set an appropriate default based on the value of $rtl.

To correct this, we create a 4th variable, $default-notification-padding, which is populated by @if $rtl, then set the default (replaceable) value of $notification-padding with this new variable.

Tradeoffs

  • Add a new variable that isn't necessarily worth documenting, and makes this definition more verbose
  • Add an insignificant amount of compute time during render

Testing Done

  • [x] Pull the latest master branch
  • [x] Make sure your Sass code is compliant with the Bulma Sass styleguide
  • [x] Make sure your PR only affects .sass or documentation files
  • [x] Try your changes.

The following code blocks are pairs of (test case bulma.sass, and the output of grep -A4 "\.notification {" css/bulma.css after running npm run build):

@charset "utf-8"
/*! bulma.io v0.9.4 | MIT License | github.com/jgthms/bulma */
@import "sass/utilities/_all"
@import "sass/elements/notification.sass"
.notification {
  background-color: whitesmoke;
  border-radius: 4px;
  position: relative;
  padding: 1.25rem 2.5rem 1.25rem 1.5rem;

Here we see the default still stays ready for ltr with a delete button on the right.

@charset "utf-8"
/*! bulma.io v0.9.4 | MIT License | github.com/jgthms/bulma */
@import "sass/utilities/_all"
$rtl: true
@import "sass/elements/notification.sass"
.notification {
  background-color: whitesmoke;
  border-radius: 4px;
  position: relative;
  padding: 1.25rem 1.5rem 1.25rem 2.5rem;

When $rtl is enabled, the padding is now updated without any other changes.

@charset "utf-8"
/*! bulma.io v0.9.4 | MIT License | github.com/jgthms/bulma */
@import "sass/utilities/_all"
$rtl: true
$notification-padding: 0px
@import "sass/elements/notification.sass"
.notification {
  background-color: whitesmoke;
  border-radius: 4px;
  position: relative;
  padding: 0px;

Now even with $rtl enabled, the output is still the specified value of $notification-padding.

Changelog updated?

No.

shreve avatar Jun 17 '22 19:06 shreve

I don't see how this is an improvement? It's simply putting the @if $rtl logic in the variable definition, rather than in the property definition?

One thing I agree on is to remove $notification-padding which isn't used anymore.

jgthms avatar Jun 17 '22 21:06 jgthms

It re-enables $notification-padding and makes its default behave sensibly, but that's only an improvement if that is the goal (which is my preference).

If your preference is to remove $notification-padding, I suppose it's not an improvement. If so, I can change this PR to simply remove it from the SASS and the documentation. The real upstream problem is that I was misled by the documentation and either of these options fixes that.

shreve avatar Jun 18 '22 17:06 shreve