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

fix: the effect of default style on custom class #85

Closed
wants to merge 3 commits into from

Conversation

kecrily
Copy link
Contributor

@kecrily kecrily commented Jun 8, 2022

The default styles that were appended to markdown-body are also appended to the user's custom classes.

Typically user specifies that wrapperClasses is intended to replace the default style, not to based on it. If they want to add some styles on the default, they can choose to keep markdown-body and add custom classes.

@yankeeinlondon
Copy link
Sponsor Collaborator

I actually don't know that we should have a hard dependency on the document's wrapper class. I think that this should operate independently of whether you want the default styles applied but I think as a short term solution it is a reasonable way to address the problem you are facing.

I am currently exploring useful ways to offer "themes" to users both at the base/core level but also when they add in Builder API's. Every stylistic theme choice needs to have a "none" option and maybe this should be the default.

@yankeeinlondon
Copy link
Sponsor Collaborator

yankeeinlondon commented Jun 16, 2022

@kecrily to document your feature exception, can you please update the failing test to show both expected outcomes?

@kecrily
Copy link
Contributor Author

kecrily commented Jun 16, 2022

I don't know why the installation of cypress under Ubuntu failed, but it should have nothing to do with me🤣

@yankeeinlondon
Copy link
Sponsor Collaborator

That's odd because before I typed the comment all tests were failing directly on the lack of base styling (which of course was directly related to your code). However, if you have time to fix Cypress while you're at it no one's going to complain :)

@yankeeinlondon
Copy link
Sponsor Collaborator

Ok, it has to do with the snapshot changing. I find snapshots very useful for developers understanding state change but completely silent on explaining expected behavior. In the code() builder here are two tests that show what I'm talking about.

  it('having a code block results in inline styles are included', async () => {
    const sfc = await composeFixture('multi-code-block', { builders: [code()] })
    expect(sfc.vueStyleBlocks?.codeStyle).toBeDefined()
    expect(sfc.component).toContain('.code-block')
  })

  it('no code block results in no in inline styles for code being being included', async () => {
    const sfc = await composeFixture('simple', { builders: [code()] })
    expect(sfc.vueStyleBlocks?.codeStyle).not.toBeDefined()
  })

@kecrily
Copy link
Contributor Author

kecrily commented Jun 16, 2022

However, if you have time to fix Cypress while you're at it no one's going to complain :)

I mean the ubuntu test workflow after I changed snapshot. See https://github.com/antfu/vite-plugin-md/runs/6913112615?check_suite_focus=true

I shouldn't be able to do anything about it

@yankeeinlondon
Copy link
Sponsor Collaborator

I will be bringing in some more testing types over the next month ... not because I love testing ... but there are some tests which are better tested after the SFC is transpiled to JS, CSS, etc. At that point I think tests like this will probably better land there but since there are changes in the pipeline I do hope you can help documenting this change you're making with a test on both sides of the configuration switch (aka, the wrapper's class name).

@yankeeinlondon
Copy link
Sponsor Collaborator

I shouldn't be able to do anything about it

No of course not. I was joking about you fixing Cypress issues. :)

@yankeeinlondon
Copy link
Sponsor Collaborator

Note: I re-ran the failed Linux job as I believe it was just a timeout and it's run successfully.

@azrikahar azrikahar mentioned this pull request Jun 17, 2022
8 tasks
@yankeeinlondon
Copy link
Sponsor Collaborator

@kecrily in the latest code I just pushed to main there is now a option: { style: { defaultStyle: 'github' }} that you can set to get some default styling but by default the style is set to "none" which has zero styling. Based on that I think this PR is no longer needed but feel free to re-open if you disagree.

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