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

Stylesheet parser should accept CSS classes without elements #203

Closed
wants to merge 1 commit into from

Conversation

cr-solutions
Copy link

Currently the "Stylesheet Parser" only accept CSS Classes with element like "p.mystyle", but not classes without elements like .table_darkgrey.

CSS Sample:
.table_darkgrey {
background-color: #a8aeb3;
border-bottom: 30px solid #a8aeb3;
border-top: 30px solid #a8aeb3;
}

For more see also the ticket https://dev.ckeditor.com/ticket/9922
It contain also my last plugin.js for the "Stylesheet Parser"

The changes have inline comments with ticket refernce.

@Reinmar
Copy link
Member

Reinmar commented Jul 27, 2015

Now the PR includes what it should, thanks.

However, there are a couple of issues:

  1. Incorrect code style and lack of tests. Read more in http://docs.ckeditor.com/#!/guide/dev_contributing_code
  2. The patch touches two parts of code. First is this loop which perhaps is the place that must be fixed. Second is the timeout added later which I don't understand at all. It looks like some legacy code that isn't necessary any more. Please always limit your patches to the minimum.

The last thing is... I don't think this is the right approach. Why using <span> by default if element name is not defined? As Alfonso commented the problem lays in the styles system which does not handle generic styles. This means styles which do not have element names defined, so which can be applied to all elements. This is a much bigger feature to code, but this is the only reasonable change that can be do. Defaulting to <span>s is just a hack which solves a limited number of cases.

Therefore, I'll to close this PR as a proper patch would not have anything in common with this. I'm also afraid that adding the mentioned feature is very complicated because it would be a pretty deep change in the styles system.

@Reinmar Reinmar closed this Jul 27, 2015
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

Successfully merging this pull request may close these issues.

None yet

2 participants