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

feat: js widgets #22

Merged
merged 11 commits into from
Aug 28, 2019
Merged

feat: js widgets #22

merged 11 commits into from
Aug 28, 2019

Conversation

lottamus
Copy link
Contributor

@lottamus lottamus commented Aug 26, 2019

Examples

HTML: cd examples/html && serve
Angular: cd examples/angular && yarn && yarn start

To Do

  • Scope CSS (ui-kit css is changing body)
  • Optimize vendor bundle (refractor is adding uneeded langs)

image

Screen Shot 2019-08-26 at 5 06 17 AM

@lottamus lottamus requested a review from marbemac August 27, 2019 20:51
@lottamus lottamus marked this pull request as ready for review August 27, 2019 20:52
@chris-miaskowski
Copy link

Hey @lottamus just found this PR quite randomly. I'm concerned about the size of this change - 17k new lines. Is it a result of a hackathon?

Some questions:

  • Can it be broken down into smaller PRs? It will be very hard (impossible) to review this otherwise.
  • Curious about the test coverage too, did you had a chance to cover this sufficiently or is it mostly code?

General thought. I noticed several very big PRs like this in the past and I'm eager to hear what your thoughts are. For example we can't actually expect anyone to review that thoroughly and I' not sure we should feel comfortable adding so much code with reviews and coverage.

And seeing that @marbemac is a single reviewer that's alarming too. Should we get at least one, two more people to help reviewing that? My concern/guess is that the scope of this work is not transparent to other folks besides @marbemac and @lottamus.

@marbemac @philsturgeon @lottamus @wmhilton @P0lip , wdyt ?

@philsturgeon
Copy link
Contributor

@chris-miaskowski are you concerned about the total number of lines in the diff, or are you concerned about where those lines are going?

Adding JS Widgets is going to be important, its not a hackathon project its a documentation integration strategy for our clients who don't want "free hosted docs" on https://stoplight.io/p/docs/

@lottamus can we avoid webpack and postcss and sass and all that going in the main yarn.lock? I suppose if its devDependencies we've not got any particular differences in the actual built asserts, so this 17k might not actually matter all that much?

@lottamus
Copy link
Contributor Author

@chris-miaskowski @philsturgeon please review the PR before commenting.

Majority of the “lines changes” are a result of the Angular example needed for Dell.

Webpack is necessary for bundling the widgets so it is added as a dev dep and represented in the lock file. It will not be included in the build output.

@philsturgeon
Copy link
Contributor

@lottamus I read the PR before commenting. Not sure why you're replying to us both like that like we were saying the same thing?

I was saying the biggest possible impact on the repo other than lots of stuff being in an examples folder which doesn't matter, is that we have grown our dependencies quite a lot.

I asked if we can avoid the dependencies in the main package.json because if any of it is required for the examples it can be moved into the examples package.

@P0lip
Copy link
Contributor

P0lip commented Aug 28, 2019

@lottamus
Granted you used ng-cli for bootstrapping, perhaps let's just have 2 PRs or commits.
The first one containing changes made by ng new and the other one(s) containing your own changes. This would greatly simplify the process of reviewing.
Either way, as Phil said, there is a lot of bootstrapping taking place, so the diff will inevitably be huge.

I can take a look at it tomorrow if you don't mind.

BTW is it a kind of example/dev code or an actual production code? To me it looks like just a plain demo that is not meant to be shown to anyone besides Dell / other potential clients, but just double-checking.

@P0lip
Copy link
Contributor

P0lip commented Aug 28, 2019

Optimize vendor bundle (refractor is adding uneeded langs)

Urgh, is actually code-viewer exported from main entrypoint? I don't think we use refactor for anything else. If that's the case, perhaps let's just remove that re-export.

Extra question, where does esprima come from? 🤔 I don't recall any project that would make use of that parser, so it must be our dep. The question is - why do we actually need it?

@lottamus
Copy link
Contributor Author

All changes to the package.json are necessary for the JS widget bundling. They are dev deps so won’t be included bundled into the output.

The Angular and HTML examples are to display how to use the widgets in both of those contexts. Purely for demo / documentation

Copy link
Contributor

@marbemac marbemac left a comment

Choose a reason for hiding this comment

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

Took over and finished because @lottamus is on vacation, and we have a deadline with Dell.

In reality, this PR has ~100 lines of actual code changes (everything else is the new examples and webpack / config additions). Of those 100 lines of code, ~90 are completely new in the widgets.tsx file, and don't affect anything else in elements.

I propose we tackle css scoping, and slimming down the widget bundle, in separate PRs - neither of these blocks Dell getting started.

This is a safe PR and feels solid, great work @lottamus! Enjoy Scotland :).

@marbemac marbemac merged commit 640e1b8 into master Aug 28, 2019
@marbemac marbemac deleted the feat/js-widgets branch August 28, 2019 23:42
@marbemac
Copy link
Contributor

Extra question, where does esprima come from? 🤔 I don't recall any project that would make use of that parser, so it must be our dep. The question is - why do we actually need it?

@P0lip it's coming from js-yaml :(

@stoplight-bot
Copy link
Collaborator

🎉 This PR is included in version 2.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

6 participants