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

Add a plan display component that can be used independently #231

Merged
merged 2 commits into from
Jul 2, 2019

Conversation

Minivera
Copy link

@Minivera Minivera commented Jul 2, 2019

This component can be used by platforms to display a plan's information if require. This can power resource details page or custom product display pages.

This component can be used by platforms to display a plan's information if require. This can power resource details page or custom product display pages.
@Minivera Minivera self-assigned this Jul 2, 2019
@drwpow drwpow temporarily deployed to manifold-ui-stage-pr-231 July 2, 2019 17:25 Inactive
@codecov
Copy link

codecov bot commented Jul 2, 2019

Codecov Report

Merging #231 into master will decrease coverage by 0.39%.
The diff coverage is 41.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #231     +/-   ##
=========================================
- Coverage   53.54%   53.15%   -0.4%     
=========================================
  Files          32       33      +1     
  Lines         706      730     +24     
  Branches      163      164      +1     
=========================================
+ Hits          378      388     +10     
- Misses        326      340     +14     
  Partials        2        2
Impacted Files Coverage Δ
src/components/manifold-plan/manifold-plan.tsx 41.66% <41.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 090ff76...225ed0a. Read the comment docs.

/**
* _(hidden)_ Passed by `<manifold-connection>`
*/
'connection': Connection;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we’re inconsistent, but this should be marked as optional. Reason is that within TypeScript projects, it’ll throw an error otherwise (connection is internal, so users shouldn’t have to specify that attribute).

Like I said there may still be some spots where that isn’t the case, but just something to note.

Copy link
Contributor

Choose a reason for hiding this comment

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

Er—whoops! I meant in manifold-plan.tsx. Not here; this is the auto-generated file.

planSelector.planChange(newPlan);
expect(planSelector.fetchProductAndPlan).toHaveBeenCalledWith(productLabel, newPlan);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! In general, we prefer E2E tests—and tests that don’t raise coverage—over spec. Spec is great for certain things, but in general web components being largely HTML are very hard to test & raise coverage in the same way you can with React (there’s no importing; it’s all just HTML).

So all that said, coverage isn’t as big a goal with UI as E2E tests w/ Puppeteer

Copy link
Contributor

@drwpow drwpow left a comment

Choose a reason for hiding this comment

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

Nice! Great first PR! Docs look great, too. 🚀

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.

2 participants