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.

Sign up for GitHub

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

Jump to bottom

Stylemark's backwards compatibility #226

Closed
mbehzad opened this issue Apr 11, 2024 · 3 comments
Closed

Stylemark's backwards compatibility #226

mbehzad opened this issue Apr 11, 2024 · 3 comments

Comments

@mbehzad
Copy link
Contributor

mbehzad commented Apr 11, 2024

compatibility of the new Stylemark with the old one (stylemark and the old pv-stylemark)

  • assets option
    lsgAssetsSrc was removed with the note: "... pv-scripts users can just use the resources folder". The issue with that is that the resource directory is meant to be part of the clientLibraries and will be deployed to the production. And shouldn't contain styleguide specific content.
    PR Stylemark's backwards compatibility  #226 will add the stylemark's assests option back, which can be used for any icons, css, etc that is needed only for the styleguide.

  • lsg and click dummy components output are combined
    the previous pv-stylemark created two outputs for each component, one standalone with header, body etc. and one only had the component markup and was then wrapped by the stylemark. The issue with only creating one set of files is that for example tests can't be run on the standalone component html files anymore. Which means every test case that doesn't necessarily need to be showcased in the styleguide, still would need to be created by it (and be hidden with the hidden attribute). The downside to it is that it is more complex, the user would need to now the naming pattern used by stylmark and have to add stuff to the readme that is only meant for the tests and can lead to confusion on the long run.
    A simple solution can be to re-enable the lsgTemplatesSrc etc and generate two files.
    Or have stylamrk to check if the html already has <html> element and in that case skip rendering the headHtml etc. And only inject the css/js from the code blocks. (and have some magic comments in the html to find the component's markup for the preview option)
    Assuming stylemark is going to be used only in combination with assemble-lite (as is currently the case in pv-stylemark), then i would argue that assemble(-lite) is a much more capable tool when it comes to generating markup than stylemark, and offloading the task as much as possible to assemble only makes sense. (the html generation by stylemark can still be used for the rare case of example's html code in the markdown file.)

(It might also be an option in the future to fully integrate the assemble in stylemark and have a MDX like files:

---
name: Button
---

example of a button 

{{> e-button
    type="primary"
    label="label"    
}}

with icon

{{> e-button
    icon-id="icon-mail"
}}

but not sure if that has a better DX compared to the current dedicated components hbs files approach)

  • support for all kind of executable code blocks
    the original stylemark would allow code blocks for html, js and css. and the content could be in the fenced code block or referenced via a file path.
    the new stylemark porivdes suport for referenced html file, embeded css styles, PR feat(pv-stylemark): add suport for js and html executable code blocks support the hidden attribute #224 will additionally add support for embedded html content and embeded js.
    what would still be missing is referencing css and js files. There can be a valid use case e.g. some lsg components might want to share some css/js for the styleguide presentation (mocking login state, etc) or loading core componets js/css that won't be deployent and therefor no need to bundle/preprocess it. (we might want to gather feedback if developers need this before spending time adding the support)

  • Theming
    The original stylemark provided suport for custemization of the lsg ui via the Theming option where specific css classes where provided for styling. PR feat(pv-stylemark): add theme classes from the old stylemark to the new one #221 adds these classes to the new UI.

  • match option
    the original stylemark provided a configuration to select which files need to be parsed for the styleguide content. This would allow: 1- having the content even in .scss files. which i don't think anyone would want to use. but 2- define a direcotry other that componentsSrc for the source of the markdown files. Although that would enable to document and showcase the pages in the stylemark ui, I don't think people really need it. But it probably should be documented what the config options of the new stylemark are, because currently only the documentation for the original stylemark can be used. (same for excludeDir option)

cheers
@mbehzad mbehzad mentioned this issue Apr 11, 2024
Closed
@friewerts
Copy link
Member

@mbehzad Thanks for your work so far.

Regarding the asset comment:
I think our goal with pv-stylemark should not be to recreate all the features of stylemark, but only the relevant ones. In my opinion, an integrated asset copy task is not one of them. The comment about using the resources folder was meant in a non aem related scenario. If you don't want the lsg-assets in your prod artefact you could very easily create a separate copy-task in your dev-config.

I think it's a good idea to concentrate on the basic features first to keep this package as simple and maintainable as possible.

@friewerts
Copy link
Member

@mbehzad since you brought up multiple good point that are not necessarily connected, i think it would be nice to track them in separate issues. this would make the discussion on the respective issues easier. what do you think?

@mbehzad mbehzad mentioned this issue Apr 15, 2024
Closed
@friewerts
Copy link
Member

closing this one since separate issue for the point exist now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@friewerts @mbehzad