Open antoniozzo opened 9 years ago
Thank you. This is a bug in our KSS block detection. We'll look into it.
+1
@hannu @cyberixae,
I'm taking a run at this bug. Let me know if I should be coordinating with someone on your core team regarding the following findings and choices.
Kss-splitter identifies a kss block by checking the Styleguide reference for digits only. This is a simple RegExp fix to allow alpha as well. Seems low risk and obvious.
Next up is the issue of sorting the sections. Below you'll find my assessment of the kss-node sorter, and the options we have in SC5 for achieving the same sort outcome.
Kss-node makes some assumptions about the section sort in KssStyleguide
weight
parameter in a kss comment overrides normal alpha sort when Styleguide references are strings.autoincrement
Taking each assumption in turn,
Access to all kss sections - SC5 sends only one kss block / file to the kss-node parser at a time. Having violated the first assumption above, the kss-node sorter can't produce the right results for SC5.
Weight parameter - the kss-node sorting algorithm modifies a standard alpha sort by using the weight
parameter. This prevents us from using an off-the-shelf sorting library. Re-using the kss-node sorter would be ideal.
Autoincrement reference - while tempting to use the .autoincrement
value for our own purposes, it has the flavor of an internal API (use at own risk), and is also inaccurate without having all sections available at once (see the first assumption.) Also, kss-node leaves the reference intact and stores a supplementary autoincrement property; we need the reverse to keep the SC5 codebase refactoring to a minimum (store the original string reference as a supplementary property, and replace the reference with the derived number.)
At base, we can either try to use the kss-node sorter by one of several methods, or write our own. Re-using kss-node seems the best option, it's just a matter of how.
Package up SC5's section list late in processing, re-construct the data structure KssStyleguide expects from its own internal parser, and let kss-node work some magic.
Pros
weight
parameter modificationsCons
Copy and modify the kss-node algorithm in the SC5 codebase. I've identified a reasonable replacement vector in SC5 is the .bySectionReference
sort comparator.
Pros
.weight
parameter override for string-based Styleguide referencesCons
I'm pursuing the second option above, "Copy and maintain". Let me know if you have other alternatives in mind.
Hi. Nice to see that you are already very familiar with the current code base.
I would go with the "Copy and maintain" option. Since the sorting logic should easy enough to implement and maintain, we do not need to replicate too much internal kss-node logic. If we would choose to pass all KSS blocks at once to kss-node it would require huge internal changes and we still need to split the KSS blocks to find related CSS and variables.
Even with the "Copy and maintain" option, we still support all other KSS features that are not dependent on other sections.
Kind of unrelated note, but good to know: Currently we use modules/kss-additional-params.js
to parse additional parameters in the KSS blocks. This module also cleans up params from the KSS markup before passing block to the kss-node. As far as I know, the current version of kss-node supports additional parameters and this module is not needed anymore and should be removed at some point.
Thanks, @hannu.
I've made good progress, and am chasing down some unit test failures (regression) when multiple files are involved. Still on the copy and maintain path, and already encapsulated the kss-node sort algorithms in a KssNodeSorter class.
A couple of questions arise around intended behavior...
Currently this causes a failure when numeric references are used. This seems a reasonable response, since reshuffling the references is both difficult and unexpected especially since the resulting sections wouldn't match the source. With string references, it would be possible to continue assigning derived numerics across duplicate references. This introduces a behavioral inconsistency between the use of numerics v. strings, but perhaps a useful one?
It seems the SC5 UI has high expectations of numeric references, which is why I'm converting string references to the derived numerics. Doing so, however, makes supporting both numeric and string references in the source kss for a single styleguide problematic. Because there is no attempt to avoid existing numeric references when deriving numerics from the string references, the chance of duplicating the original numerics is almost assured. (Kss-node doesn't encounter this problem since the original references are used directly in their styleguide, rather than the derived numerics. Essentially, the original numerics are considered strings in such a "mixed mode.")
Some options,
Thoughts?
+1
+1
+1
+1
+1
+1
+1
+1
+1
+1
+1
+1
+1
+1
+0
+1
+1
+1
+1
Hello everyone,
I have already a branch, in which I am preparing this feature. In the mean time, instead of adding more comments with +1 would you add a reaction to the first comment, with the +1 icon version?
It would help a lot to keep the ticket clean.
I think the SC5 template should support strings as section references:
Numbers are hard to maintain if you keep moving stuff around or adding. This is something that the kss-node template have.
If this is already implemented but not documented, please tell me how to do it.