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

Deprecate periods in staticSearch classes #149

Closed
joeytakeda opened this issue Apr 10, 2021 · 11 comments
Closed

Deprecate periods in staticSearch classes #149

joeytakeda opened this issue Apr 10, 2021 · 11 comments
Labels
enhancement New feature or request release-blocker The dev branch must not be merged into master while a release-blocker bug is open.
Milestone

Comments

@joeytakeda
Copy link
Contributor

As @martindholmes notes in #84:

But this does bring up for me the issue of using periods in class names; it's not ideal really, because of the class-chaining syntax in selectors. I wonder if we shouldn't be doing that?

[...]

With 1.2 we did pilot a pretty decent deprecation method, which we could also follow to fix the period issue by supporting both in the next version, but warning when period values are found. I do think it's worth doing.

I agree that it's worth doing. A few questions:

  • What should the periods be replaced by? Should we simply move to camel case (i.e. staticSearch.desc -> staticSearchDesc) or should we make all of the classes align with the ssTYPE syntax that we use for the filter JSONs etc? (i.e. staticSearch.desc -> ssDesc)?

  • For 1.3, I think we should have a template in the document parsing that finds any staticSearch. and converts it to the syntax we decide with a large WARNING. For 1.4, we could change that warning to an ERROR.

  • We'll also have to go through our CSS and JS to make sure all of the default styling uses the proper classes etc.

@joeytakeda joeytakeda added enhancement New feature or request release-blocker The dev branch must not be merged into master while a release-blocker bug is open. labels Apr 10, 2021
@joeytakeda joeytakeda added this to the Release 1.3 milestone Apr 10, 2021
@martindholmes
Copy link
Collaborator

I responded to this from my phone but nothing got through, so I'll try again:

I think I'd like to keep the full staticSearch term in the classNames because it helps clarify the purpose of those meta elements for anyone reading the code of the pages. I'm fine with underscores or camelCase.

I don't think we can convert any existing syntax in the user's own pages, because we're not supposed to be changing them at all (with the exception of the search page itself, of course). I think it's fine to warn about the problem, though.

Another option would be to make the next version 2.0, as an additional flag to say that there are significant changes. We can still have elegant deprecation and warning, but we could remove that from a future version.

@joeytakeda
Copy link
Contributor Author

I think I'd like to keep the full staticSearch term in the classNames because it helps clarify the purpose of those meta elements for anyone reading the code of the pages. I'm fine with underscores or camelCase.

That sounds fine with me—it'll be easier to switch, too. Let's do underscores: it will be trivial for people to change if they need to and it will make the internal switch easier too.

I don't think we can convert any existing syntax in the user's own pages, because we're not supposed to be changing them at all (with the exception of the search page itself, of course). I think it's fine to warn about the problem, though.

We would do this in the tokenizing templates when we're passing through the documents and removing stuff, etc. So during the clean up stage, we would switch all of the staticSearch.desc to staticSearch_desc; that way, our processing only has to handle the new, good variant and we can simply drop that conversion and turn it into an ERROR that terminates for next version.

Another option would be to make the next version 2.0, as an additional flag to say that there are significant changes. We can still have elegant deprecation and warning, but we could remove that from a future version.

That's true; I guess we haven't really set parameters for what constitutes a major version versus a minor one. I suppose the addition of a new filter type (in #84) could justify bumping up to 2.0.

@joeytakeda
Copy link
Contributor Author

Started a new branch for this in iss149-deprecate-periods

@martindholmes
Copy link
Collaborator

Ah, I see what you mean now -- not changing the actual documents, just changing the cleaned-up versions. Makes perfect sense.

joeytakeda added a commit that referenced this issue Apr 11, 2021
… fail, which is expected (though a bit mysterious to me.)
joeytakeda added a commit that referenced this issue Apr 11, 2021
joeytakeda added a commit that referenced this issue Apr 12, 2021
joeytakeda added a commit that referenced this issue Apr 12, 2021
…hen fixing up some documentation while I was in the file.
@joeytakeda
Copy link
Contributor Author

As far as I can tell, branch iss149-deprecate-periods is now ready to be tested on some projects. I'm going to test on WEA soon (not today, but hopefully tomorrow).

Summary of changes:

  • tokenize.xsl now investigates all of the <meta> tags (during the #clean template) that have a class that contains (^|\s+)staticSearch\. , warns that the "staticSearch." syntax is deprecated and advises using "staticSearch_," and then replaces the "." with a "_"
  • json.xsl has been slightly modified to handle the new syntax, but since we're using grouping, it only meant changing it in a few places
  • makeSearchPage.xsl puts the staticSearch_ class alongside the old staticSearch. syntax so that any existing CSS or JS will continue to work for the time being
  • StaticSearch.js uses the staticSearch_ values and uses the standard . class chaining notation in order to retrieve the appropriate input
  • split_documentation.xsl now uses the "staticSearch_" syntax, as do all of the test files with the exception of badthings.html.

@martindholmes
Copy link
Collaborator

Thanks Joey! I'll switch it into a couple of projects and we'll see what happens.

@martindholmes
Copy link
Collaborator

I've kicked off builds of Keats and Graves to test the branch.

@joeytakeda
Copy link
Contributor Author

Great! One interesting thing to note per our earlier conversation about the CSS: I didn't change any of the staticSearch CSS, since it didn't have any rules for the staticSearch. classes. And between the LOI and WEA CSS (which both contain a fair number of rules), there was only a single rule that used a staticSearch. class.

@martindholmes
Copy link
Collaborator

Graves and Keats both appear to be unchanged. I would say you can merge this into dev and we'll deal with the fallout in other projects. I expect MyNDIR will have some things to fix, but that build is broken right now anyway by something unrelated.

@joeytakeda
Copy link
Contributor Author

joeytakeda commented Apr 13, 2021

This is now merged into dev. Everything should be a WARNING, so all of the Jenkins projects will hopefully just be unstable until they use the new syntax. I'll update the documentation in dev as well.

joeytakeda added a commit that referenced this issue Apr 13, 2021
…cs/search to the gitignore (and removing the cached files), which seemed to slip by in #142
@martindholmes
Copy link
Collaborator

I think this is all good now, so closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request release-blocker The dev branch must not be merged into master while a release-blocker bug is open.
Projects
None yet
Development

No branches or pull requests

2 participants