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

Export themes #307

Closed
wants to merge 5 commits into from
Closed

Export themes #307

wants to merge 5 commits into from

Conversation

alexjeffburke
Copy link
Member

@alexjeffburke alexjeffburke commented Dec 29, 2019

This PR alters unexpected-dom to export the supporting styles corresponding to its exported assertions and types.

The rationale behind the implementation is to work within the following constraints:

  • magicpen-prism is a magicpen plugin as well as an Unexpected plugin and should continue to function that way
    • -> it is the unexpected-dom that provides these facilities to its parent, therefore exporting all pre-requisites for its operation should remain its responsibility
  • Unexpected currently manages children, and is the only component aware of "child" instances
    • -> for this reason, exportTheme() should remain a concept for Unexpected only

This PR depends on:

@coveralls
Copy link

coveralls commented Dec 29, 2019

Pull Request Test Coverage Report for Build 1309

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 93.502%

Totals Coverage Status
Change from base Build 1301: 0.0%
Covered Lines: 690
Relevant Lines: 720

💛 - Coveralls

src/index.js Outdated
@@ -268,6 +268,10 @@ module.exports = {
expect = expect.child();
expect.use(require('magicpen-prism'));

// export the items registered by magicpen-prism to callers
expect.exportTheme('html', expect.output.theme('html'));
expect.exportStyle('code', expect.output.styles.code);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is a bit awkward. I wonder if we should make theme-exporting a magicpen feature?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@papandreou hm I’m not sure it makes sense for magicpen - it doesn’t have a concept of children, so we’d end up with it having an exportTheme method being synonymous with installTheme which Unexpected core would overwrite to treat specially.

I tried to note down the constraints that played in here in the description, but to summarise exporting is an Unexpected only concept and my sense is it should remain so. I’d also note exportStyle already works this way, so it seemed consistent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it's nasty if every plugin that uses magicpen-prism internally has to do this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@papandreou I see what you're getting at - may have a suggestion.

@alexjeffburke
Copy link
Member Author

@papandreou I had a small brainwave that might allow us to address this with less nastiness in each assertion - let me know what you think of: https://github.com/unexpectedjs/unexpected-dom/pull/307/files#diff-1fdf421c05c1140f6d71444ea2b27638R270

Basically, I propose an additional exportPlugin() method - what this does, is given any plugin it causes all the bits of it to be exported. Implenentation in core is here: unexpectedjs/unexpected@fac75c7

@alexjeffburke
Copy link
Member Author

Superseded by #309.

@alexjeffburke alexjeffburke deleted the feature/exportTheme branch December 31, 2019 00:34
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

3 participants