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

Duplicate class names are removed by Meteor #1526

Closed
williamkhshea opened this issue Dec 29, 2014 · 36 comments
Closed

Duplicate class names are removed by Meteor #1526

williamkhshea opened this issue Dec 29, 2014 · 36 comments

Comments

@williamkhshea
Copy link

It is not an issue of semantic-ui per se. I am using Meteor with semantic-ui and trying to make my columns responsive for device like this:

<div class="ui grid">
  <div class="sixteen wide mobile eight wide tablet four wide computer column">
    <p></p>
</div>

Meteor removes all the duplicated class "wide" and it becomes:

<div class="ui grid">
  <div class="sixteen wide mobile eight tablet four computer column">
    <p></p>
</div>

Just wonder do you have any workarounds?

@anestv
Copy link

anestv commented Dec 29, 2014

I imagine that this will impact other word-order dependencies as well. Example:

<div class="ui right floated right labeled icon button">

@HenrikJoreteg
Copy link

So am I understanding correctly that this actually breaks Semantic UI?

Because if so, it's not just a framework-specific issue, even the browser Native classList.add does the same thing: https://developer.mozilla.org/en-US/docs/Web/API/Element.classList

@williamkhshea
Copy link
Author

@HenrikJoreteg Yes. Fortunately word-order dependent duplicate class names are not used extensively in semantic-ui.

@HenrikJoreteg
Copy link

Ok, thanks for the heads up, I was not aware of this. In the cross-linked issue above, we will not support significant word-orders or duplicate classes. Especially given how the native APIs work.

@Anachron
Copy link

@HenrikJoreteg @williamkhshea I was just thinking, a new child element with the remaining classes should totally fix it, shouldn't it?

@jlukic Is there any case where it doesn't?

@jlukic
Copy link
Member

jlukic commented Jan 12, 2015

I've left some comments over in the other discussion AmpersandJS/amp#52 (comment)

I think dupe removal should be an optional feature of APIs that add/remove class names, with the default of being enabled. The amount of SUI that is 'word order dependent' is very small. So only in certain cases would it be necessary for adjustments with mutator methods.

@stephannv
Copy link

I've solved with clean javascript:

Template.myTemplate.rendered = function() {
    document.getElementById('myDiv').setAttribute( 'class', 'ui sixteen wide tablet only sixteen wide mobile only ten wide computer only column');
}

@fentas
Copy link
Contributor

fentas commented Jun 29, 2015

Is there any update on this?
My solution for now is

$(document).ready(function() {
    setTimeout(function() {
        $('*[classs]').each(function() {
            $(this).attr('class', $(this).attr('classs'))
        })
    }, 0)
})

Means that I just rename
[...] class="... wide ... wide" [...] to [...] classs="... wide ... wide" [...].
But I would like to have an official way to do it.

@teejayhh
Copy link

I dont have any solution but is this issue being worked on ? I use semantic ui and i need a class definition like - "ui middle aligned center aligned grid" which gets invalidated when use the FlowRouter /FlowLayout . Interestingly this only happens when I use a layout + template.

the solution above is ok but I would it combine with @stephanngamedev solution which is a bit neater in a meteor context.

@zhubofei
Copy link

This should be a design flaw.

@ianmartorell
Copy link

I'd also like to get an official fix on this, the methods described above seem pretty hacky to me. I don't know if this is related, but the doubling class is not working for me either.

@jlukic
Copy link
Member

jlukic commented Jul 27, 2015

@ianmartorell If you're having specific issues with doubling, please create a test case and a new thread.

This bug is specific to how each framework implements their version of managing class names. Ideally frameworks would allow specifying space separated names for class names that should appear together.

For example @HenrikJoreteg was gracious enough to support space separated class names in Ambersand JS implementation of addClass, making it easier to ensure 'word order significant' class names are added together in the same function call. However, even this approach will still have issues when one of the class names is already in classList, for example changing from right floated left aligned to right floated right aligned might get you right floated aligned

For me the use of significant class order is an ideological stand. I think that programming languages will come to see class names as sharing similar semantic features to words, where meaning is often derived from context. Otherwise as programmers how could we possibly define "right"ness, without acknowledging the importance of contextual significance, like "right aligned" or "right floated". In most natural languages this contextual significance is expressed through word order, and I admit the implementation of that currently in javascript is somewhat premature.

I think one possible working solution moving forward is to support both hyphenated class names and word order significant class names for, at least to start, the most commonly used variations, like four wide column three column grid. The caveat though is this will add a lot of bulk to the css, and will take significant restructuring.

@ianmartorell
Copy link

@jlukic I completely agree with you, and I think in this case Meteor is at fault in removing duplicate classes as there is no real benefit to it and many frameworks use them. How semantic would sentences be if we removed all duplicate words in them? :) Has anyone filed a bug report or opened an issue on this matter with Meteor? Maybe that should be the first step forward.

@ianmartorell
Copy link

I also tried to implement hyphen-separated classes for the ones I was using in my project, but I couldn't get it to work properly.

@import (multiple) '../../theme.config.import.less';

@media only screen and (min-width: @tabletBreakpoint) and (max-width: @largestTabletScreen) {
  .ten-wide-tablet {
    width: @tenWide !important;
  }
  .eight-wide-tablet {
    width: @eightWide !important;
  }
  .six-wide-tablet {
    width: @sixWide !important;
  }
}

@media only screen and (min-width: @computerBreakpoint) {
  .five-wide-computer {
    width: @fiveWide !important;
  }
  .four-wide-computer {
    width: @fourWide !important;
  }
  .three-wide-computer {
    width: @threeWide !important;
  }

@xtfer
Copy link

xtfer commented Jul 28, 2015

Both Meteor and Semantic UI are freely interpreting the HTML5 spec (http://www.w3.org/html/wg/drafts/html/master/dom.html#classes), not always usefully. The spec notes that classes are a set of space-separated tokens.

Meteor is incorrect in removing duplicates, in that the HTML5 spec doesn't specify that the class set should be unique.

Semantic UI is adding a constraint on the use of classes by requiring class names to be a non-unique ordered set, a type of set not defined in the specification. Common understanding of class usage is that individual tokens have the same meaning in the same context – the DOM element they refer to – as tokenisation in the HTML spec by definition cannot group tokens. Ordered token groups is really asking for trouble.

@jlukic
Copy link
Member

jlukic commented Jul 28, 2015

@xtfer That is all explaining constraints as defined by spec, created by W3C. This is equivalent to making a legal argument in a debate over moral philosophy. I'm not disputing that my approach is abnormal given current programming norms.

My believe is that these accepted programming norms are unusual constraints on conveying meaning, and that any simple demonstration given other developed systems of conveying meaning (read natural languages) will show that most meaning cannot be interpreted without significant word order.

@undercase
Copy link

I'm unsure of what the problem is here, I'd say this is a problem with Meteor more than it is a problem with Semantic UI - the alternative would seem to make Semantic UI less semantic, simply because one framework doesn't follow the W3C specification.

@phrone
Copy link

phrone commented Jul 28, 2015

Hey, I wrote a quick and dirty workaround function to last me until the problem gets resolved. Feel free to use it if you like the approach. Slap it in your client lib and call it from your template rendering function. Performance and elegance haven't been taken into consideration - I just didn't feel like writing duct tape CSS.

/**
* Makes sure the positional 'wide' class is present for every device-specific semantic-ui rule
*/
fixSemanticGrids = function () {
  var deviceList = ['computer', 'tablet', 'mobile'];

  $('.wide.column').each(function (i, e) { 
    var classes = $(e).attr('class').split(/\s+/);
    var newClasses = new Array();
    $.each(classes, function (j, c) {
      if($.inArray(c, deviceList) >= 0 && j > 0 && 'wide' != classes[j-1]) {
        newClasses.push('wide');
      }
      newClasses.push(c);
    });
    $(e).attr('class', newClasses.join(' '));
  });
};

@ianmartorell
Copy link

Thanks @phrone! That works quite well for now.

@xtfer
Copy link

xtfer commented Jul 28, 2015

@jlukic No it isn't. Code is not moral philosophy. When you ignore the spec you get Internet Explorer. My point, however, was that if you freely interpret the specification in a way that is different to how everyone else does, at some point you are going to run in to problems.

In any case, on reviewing it again, Semantic UI does actually break the spec, Meteor is correct.

"The classes that an HTML element has assigned to it consists of all the classes returned when the value of the class attribute is split on spaces. (Duplicates are ignored.)".

EDIT: Added reference

@jlukic
Copy link
Member

jlukic commented Jul 28, 2015

I'm going to go ahead and stop this conversation here to avoid any unnecessary internet fighting. I've made the ideological goals of this library clear previously. If you haven't yet, check through my previous posts in GitHub issues or listen to my changelog podcasts to understand my perspective on the W3C and programming.

If anyone has any specific contributions on how to maintain support for libraries that modify repeated class names feel free to discuss this, other posts will be moderated.

@sjayendran
Copy link

thanks @phrone! that worked perfectly!! great concise interim fix! 👍

@luisreyes
Copy link

Fix

  1. Go to \semantic-ui\definitions\collections\grid.import.less
  2. Do a search and replace:
    • Search for [space]wide[space]
    • Replace with -wide-

_OLD_

Original file with separate classes

.ui.grid > .row > [class*="sixteen wide tablet"].column,
.ui.grid > .column.row > [class*="sixteen wide tablet"].column,
.ui.grid > [class*="sixteen wide tablet"].column,
.ui.column.grid > [class*="sixteen wide tablet"].column {
    width: @sixteenWide !important;
}

_NEW_

Updated file with one class separated by dashes

.ui.grid > .row > [class*="sixteen-wide-tablet"].column,
.ui.grid > .column.row > [class*="sixteen-wide-tablet"].column,
.ui.grid > [class*="sixteen-wide-tablet"].column,
.ui.column.grid > [class*="sixteen-wide-tablet"].column {
    width: @sixteenWide !important;
}

Update your classes to include dashes
<div class="eight-wide-computer sixteen-wide-tablet column">

These will now be treated as a single class and Meteor will not remove any duplicate classes... since there will be none.

@AVapps
Copy link

AVapps commented Dec 5, 2015

@luisreyes 👍 works perfectly. Thanks.

@ramezrafla
Copy link

@jlukic,

The solution proposed by @luisreyes works great!

Nothing wrong with having both eight wide computer and eight-wide-computer

Is this something that makes sense for you to support out of the box?

My feeling is that it would win over more users. When I first saw the docs, I did scratch my head about duplicates of classes (specifically that our eyes are taught to get overwhelmed when there is too much on the screen, vs with the dashes, where we see 'fewer' classes).

This could be an alternate for the more 'pedantic' while the original is there for the more 'semantic'

@jlukic
Copy link
Member

jlukic commented Dec 29, 2015

I think it does make sense to support, but I need to evaluate the KB size required for me to modify the rules in grid. It might be substantial.

@ramezrafla
Copy link

@jlukic,

Thanks for taking a look.

My guess is that the increase in size won't be substantial as you are adding more selectors to the existing css rules.

Also, I don't think we NEED other such changes except maybe <device>-only and <device>-reversed to prevent duplicate device classes if we try to add more device-based selectors (I may have missed other selectors prone to conflicts, but don't believe so).

The other rules can remain as such, unless you want to make it global that we have duplicates with dashes.

@mitar
Copy link

mitar commented May 5, 2016

Meteor is incorrect in removing duplicates, in that the HTML5 spec doesn't specify that the class set should be unique.

Not true. Spec says:

The classes that an HTML element has assigned to it consists of all the classes returned when the value of the class attribute is split on spaces. (Duplicates are ignored.)

(Emphasis mine.)

And Meteor is ignoring duplicates per spec.

Anyway, I am here to report that there is a pull request on this topic made towards Blaze: meteor/blaze#37

Is there any progress on this? I think the proposal above of changing eight wide computer to eight-wide-computer seems pretty reasonable? It could maybe even be done just for the Meteor package for Semantic UI?

@ramezrafla
Copy link

ramezrafla commented May 5, 2016

Agreed, here are my notes on what I changed manually:

In ui/definitions/collections/grid.import.less
Replace [space]wide[space] --> -wide-
Replace -wide-large screen --> -wide-large-screen

@jlukic
Copy link
Member

jlukic commented May 20, 2016

Deprecations make things move slow...

@ramezrafla
Copy link

@jlukic, not sure I understand.

On another issue, I was thinking, instead of changing the CSS globally, why not only change the JS builder for Meteor. It could run the above changes during build time. This way you don't have to change it for everyone.

@talha-asad
Copy link

talha-asad commented Jun 17, 2016

The problem is not just for "wide" classes, we have "center aligned middle aligned" as well. I think the proposal to make these changes only for the meteor builder or other XYZ framework would work reasonably as it won't be adding any more bloat to the actual framework.

The classes that an HTML element has assigned to it consists of all the classes returned when the value of the class attribute is split on spaces. (Duplicates are ignored.)

This is right and per spec, however most browsers don't follow this spec, in the sense they don't remove duplicates so it was kind of bad on Meteors part to remove them. That's why I submitted that pull request to begin with.

@arggh
Copy link

arggh commented Jun 18, 2016

I fixed the issue like this for myself:

<template name="div">
  <div>
    {{> Template.contentBlock}}
  </div>
</template>
Template.div.onRendered(function() {
  $(this.firstNode).get(0).className = this.data.class;
});

How to use:

<div class="ui grid">
   {{#div class="sixteen wide mobile twelve wide tablet ten wide computer column"}}
      <!-- column content -->
   {{/div}}
</div>

@Bandit
Copy link

Bandit commented Aug 1, 2016

Would like to +1 a solution to this

@joaobarcia
Copy link

+1

@stale
Copy link

stale bot commented Feb 23, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 30 days if no further activity occurs. Thank you for your contributions.

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