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

WARNING Potentially invalid value for $container-max-widths #24061

Closed
romanlex opened this issue Sep 23, 2017 · 20 comments
Closed

WARNING Potentially invalid value for $container-max-widths #24061

romanlex opened this issue Sep 23, 2017 · 20 comments

Comments

@romanlex
Copy link
Contributor

Hello, I have warning when trying compile styles with this variables:

$container-max-widths: (
				sm: 540px,
				md: 720px,
				lg: 960px,
				xl: 92%
);
@include _assert-ascending($container-max-widths, "$container-max-widths");

Message:
WARNING: Potentially invalid value for $container-max-widths: This map must be in ascending order, but key 'xl' has value 92% whose unit makes it incomparable to 960px, the value of the previous key 'lg' !

I think what this block is wrong and should ignore the specified values in percentage

@each $key, $num in $map {
    @if $prev-num == null {
      // Do nothing
    } @else if not comparable($prev-num, $num) {
      @warn "Potentially invalid value for #{$map-name}: This map must be in ascending order, but key '#{$key}' has value #{$num} whose unit makes it incomparable to #{$prev-num}, the value of the previous key '#{$prev-key}' !";
    } @else if $prev-num >= $num {
      @warn "Invalid value for #{$map-name}: This map must be in ascending order, but key '#{$key}' has value #{$num} which isn't greater than #{$prev-num}, the value of the previous key '#{$prev-key}' !";
    }
    $prev-key: $key;
    $prev-num: $num;
  }
@mdo mdo removed their assignment Oct 18, 2017
@sabrown84
Copy link

Hi @mdo @XhmikosR @Johann-S,

I'm trying to figure out where this issue is and how to go about fixing it. Could you help me a little by pointing me in the right direction?

@romanlex
Copy link
Contributor Author

@sabrown84 function comparable from sass cannot compare percentage values with px

@sabrown84
Copy link

@romanlex

Do you think something like this will work?

@romanlex
Copy link
Contributor Author

romanlex commented Nov 2, 2017

@sabrown84 I don't known how work this function inside, but function receive only numbers

/**
 * Returns whether two numbers can added, subtracted, or compared.
 * 
 * @example
 * comparable(2px, 1px) => true
 * comparable(100px, 3em) => false
 * comparable(10cm, 3mm) => true
 * @overload comparable($number1, $number2)
 * @param $number1 [Sass::Script::Value::Number]
 * @param $number2 [Sass::Script::Value::Number]
 * @return [Sass::Script::Value::Bool]
 * @raise [ArgumentError] if either parameter is the wrong type
 */
@function comparable($number1, $number2) { /* stub */ }
Parameters:
$number1 (Sass::Script::Value::Number)
$number2 (Sass::Script::Value::Number)

If function receive Script as parameter, maybe we can write mixin for this? But precentage and integer values(px) cannot be comparable

@romanlex
Copy link
Contributor Author

romanlex commented Nov 2, 2017

universal solution for this code is remove all warnings

@each $key, $num in $map {
    $prev-key: $key;
    $prev-num: $num;
  }

🤣

@sabrown84
Copy link

yes ok I think I understand.

@romanlex
Copy link
Contributor Author

romanlex commented Nov 2, 2017

@sabrown84 Maybe we can use $grid-breakpoints for comparable.

$grid-breakpoints: (
				xs: 0,
				sm: 576px,
				md: 768px,
				lg: 992px,
				l: 1138px,
				xl: 1440px
);
$container-max-widths: (
				sm: 100%,
				md: 100%,
				lg: 100%,
				l: 1138px,
				xl: 92%
);

In this example we has $container-max-widths with sm key setted to 100%. For this case 100% always less than 560px. In xl case 92% always more than 1440px and we can understand that.

@romanlex
Copy link
Contributor Author

romanlex commented Nov 2, 2017

@sabrown84 I read libsass source and found this:
comparable function use find_convertible_unit()

Signature comparable_sig = "comparable($number-1, $number-2)";
    BUILT_IN(comparable)
    {
      Number_Ptr n1 = ARG("$number-1", Number);
      Number_Ptr n2 = ARG("$number-2", Number);
      if (n1->is_unitless() || n2->is_unitless()) {
        return SASS_MEMORY_NEW(Boolean, pstate, true);
      }
      Number tmp_n2(n2); // copy
      tmp_n2.normalize(n1->find_convertible_unit());
      return SASS_MEMORY_NEW(Boolean, pstate, n1->unit() == tmp_n2.unit());
    }

find_convertible_unit:

// useful for making one number compatible with another
  std::string Number::find_convertible_unit() const
  {
    for (size_t i = 0, S = numerator_units_.size(); i < S; ++i) {
      std::string u(numerator_units_[i]);
      if (string_to_unit(u) != UNKNOWN) return u;
    }
    for (size_t i = 0, S = denominator_units_.size(); i < S; ++i) {
      std::string u(denominator_units_[i]);
      if (string_to_unit(u) != UNKNOWN) return u;
    }
    return std::string();
  }

and last step is the string_to_unit()

 UnitType string_to_unit(const std::string& s)
  {
    // size units
    if      (s == "px")   return UnitType::PX;
    else if (s == "pt")   return UnitType::PT;
    else if (s == "pc")   return UnitType::PC;
    else if (s == "mm")   return UnitType::MM;
    else if (s == "cm")   return UnitType::CM;
    else if (s == "in")   return UnitType::IN;
    // angle units
    else if (s == "deg")  return UnitType::DEG;
    else if (s == "grad") return UnitType::GRAD;
    else if (s == "rad")  return UnitType::RAD;
    else if (s == "turn") return UnitType::TURN;
    // time units
    else if (s == "s")    return UnitType::SEC;
    else if (s == "ms")   return UnitType::MSEC;
    // frequency units
    else if (s == "Hz")   return UnitType::HERTZ;
    else if (s == "kHz")  return UnitType::KHERTZ;
    // resolutions units
    else if (s == "dpi")  return UnitType::DPI;
    else if (s == "dpcm") return UnitType::DPCM;
    else if (s == "dppx") return UnitType::DPPX;
    // for unknown units
    else return UnitType::UNKNOWN;
  }

this function don't know percantage % as units

@sabrown84
Copy link

Hi @romanlex
I'm a little lost on this one. I can't figure out what to do fix this one. Thank you for your insight, but I may need to take some time to think more about this one.

@romanlex
Copy link
Contributor Author

romanlex commented Nov 4, 2017

@sabrown84 think about what? Imho I don't see any universal method for this. I think container width may be are given as a percentage. thx for your reply

@romanlex
Copy link
Contributor Author

Hello. What you think about this issues?

@XhmikosR
Copy link
Member

/CC @andresgalante

@aendu
Copy link

aendu commented Jan 22, 2018

Afsik it is used 2 times in the bootstrap sources:

  • grid-breakpoints
  • container-max-widths

For the breakpoints i understand the implications but i don't see it for the container width.
Why should it be wrong to have the same width for example for large (lg) and extra-large (xl)?

at least the part } @else if $prev-num >= $num { should not contain the = (just speaking for the 2nd use case).

Or am i missing something?

@elsbrock
Copy link

Please don't highlight me. Thanks!

@Paladinium
Copy link

We have the same use-case as @romanlex where the container-max-width of xl is 100%. Sure, the perfect solution would be to allow percentage values. But we would also be happy with a dumb switch to entirely disable the asserts.

@Paladinium
Copy link

@romanlex : as a contributor, do you see any chance to make progress on this one? I (and others) would highly appreciate it since those warnings are spamming the build.

@TokenR1ng
Copy link

Would be great to see any progress on this one. We have the same issue in our build.

@romanlex
Copy link
Contributor Author

romanlex commented Jun 8, 2018

@Paladinium @TokenR1ng you can test this #26689

@Paladinium
Copy link

@romanlex : wow, thanks, that's really great!

@mdo mdo closed this as completed in #26689 Sep 2, 2018
@spokospace
Copy link

there is the same problem with width in "vw"

WARNING: Potentially invalid value for $container-max-widths: This map must be in ascending order, but key 'lg' has value 100vw whose unit makes it incomparable to 960px, the value of the previous key 'md' !
on line 14 of node_modules/bootstrap/scss/_functions.scss, in mixin -assert-ascending
from line 214 of node_modules/bootstrap/scss/_variables.scss
from line 9 of node_modules/bootstrap/scss/bootstrap.scss
from line 4 of stdin

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

No branches or pull requests

10 participants