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

.styleColumn Documentation / ColumnHider issues #885

Closed
PunkChameleon opened this issue Apr 1, 2014 · 17 comments
Closed

.styleColumn Documentation / ColumnHider issues #885

PunkChameleon opened this issue Apr 1, 2014 · 17 comments

Comments

@PunkChameleon
Copy link
Contributor

Hey all,

I'm working with the styleColumn function. Currently, I'm trying to loop through my columns, determine whether they are hidden or not, and style them accordingly.

My question is this -- in the (documentation)[https://github.com/SitePen/dgrid/wiki/Grid], it refers to ID -- is this the id of the column object (as I took it) or the id of the html element (for CSS)?

@PunkChameleon
Copy link
Contributor Author

In addition, what can be passed into the css parameter? Inline CSS?

The reason I bring this up, is that I'm passing the object ID followed by 'display: none;' into the function but keep getting 'Illegal string exception'. It seems to be failing on this line in addCssRule in misc.js

 extraSheet.addRule(selector, css) 

@kfranqueiro
Copy link
Member

The CSS parameter should be a string containing CSS rules, written as if you were writing them inside a CSS declaration yourself. In other words, it should be in the format property1: value1; property2: value2; ...

The id corresponds to the id of the column object, yes. By default, if id is not specified for columns, it is assigned the column's key (if a columns object was specified) or index (if an array was specified).

I've tried to clarify things a little bit on the wiki pages.

If you are trying to show/hide columns, maybe you should look at dgrid/extensions/ColumnHider? It handles adding/removing display: none declarations for you.

@PunkChameleon
Copy link
Contributor Author

Awesome thanks for explaining.

And for columnHider, that looks extremely helpful! However I'm still getting the illegal string error, but it's now being thrown in the _hideColumn function of ColumnHider. Specifically this line:

 this._columnHiderRules[id] =
            miscUtil.addCssRule(selectorPrefix + id, "display: none;");

@kfranqueiro
Copy link
Member

Out of curiosity... what's your column id? i.e. what's the full value of the first argument to miscUtil.addCssRule when it runs and throws?

@PunkChameleon
Copy link
Contributor Author

The id is:

SOME_ACCTS:SOME_ID

And the first argument passed is:

  #dijit_layout_ContentPane_4 .dgrid-column-SOME_ACCTS:SOME_ID

Could the ':' be throwing it?

@PunkChameleon
Copy link
Contributor Author

In another class, my id is `_Some Example' and it is also failing (because of the space im guessing)

@PunkChameleon
Copy link
Contributor Author

I've verified it works without the colon. Is there a low-level way to escape this before it hits the HTML layer?

@PunkChameleon PunkChameleon changed the title .styleColumn Documentation .styleColumn Documentation / ColumnHider issues Apr 2, 2014
@PunkChameleon
Copy link
Contributor Author

I think it has something to do with the fact that id is not escaped the same way as selector is. I get errors in _hideColumn and _renderHiderMenuEntry. Since in both functions they are used as a method of searching the DOM, an unescaped : is throwing it off a bit I believe.

Do you think this is the right path? Is there a lower-level to escape these since it happens both with styleColumn and dgrid/extensions/ColumnHider?

@dylans
Copy link
Member

dylans commented Apr 2, 2014

While colons are valid HTML ids, it's generally not a recommended approach because it can create ambiguity with CSS pseudo-selectors ( https://stackoverflow.com/questions/70579/what-are-valid-values-for-the-id-attribute-in-html )

@PunkChameleon
Copy link
Contributor Author

I totally understand, but unfortunately I'm not in a position to change the ids that I'm receiving / using. I thought this might be worthwhile to implement if the cost wasn't too great, as they are valid (though not preferred) selections since the HTML4 spec and others might have run into / will run into a similar issue.

Thanks again for being so responsive with this. I hope this discussion is beneficial in some way!

Technically - Would it be beneficial at all to escape the id here?

@kfranqueiro
Copy link
Member

Actually, we have had something like this come up in the past, but it looks like we only handled it for grid IDs, not column IDs. It might be trivial for us to fix this.

@PunkChameleon
Copy link
Contributor Author

That'd be amazing! Do you have any link or reference to that PR?

@PunkChameleon
Copy link
Contributor Author

Could this have something to do with it? -- #402. Am continuing to search through old PRs / issues

@PunkChameleon
Copy link
Contributor Author

Temporary fix (users can also escape it themselves before sending it into styleColumn). Doesn't fix columnHider. Tried using miscUtils.escapeCssIndentifier but the / characters caused issues as well. Where would be the lowest possible level we could escape this?

@kfranqueiro
Copy link
Member

We wouldn't want to escape it in addCssRule itself, since the first parameter is the entire selector string - and things like spaces and colons are of course actually valid parts of selectors. However, we escape the grid id specifically (e.g. 41c0362, originally in #402), and it didn't occur to me at the time to do the same for column IDs.

I'm working on that now, and am also going to try to add some tests for it this weekend, since you have pointed out a case that very easily and loudly fails in many browsers.

@kfranqueiro
Copy link
Member

I've got a branch in progress with fixes and tests. I'm still pondering some improvements to the tests.

@PunkChameleon
Copy link
Contributor Author

Great -- thank you for the updates! Excited this is getting built!

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

No branches or pull requests

3 participants