Skip to content

Conversation

@P0lip
Copy link
Contributor

@P0lip P0lip commented Aug 4, 2019

Fixes #42
Closes #45

@P0lip P0lip force-pushed the feat/worker branch 2 times, most recently from 4039773 to aaed694 Compare August 7, 2019 08:32
@P0lip P0lip self-assigned this Aug 21, 2019
@P0lip P0lip requested a review from lottamus August 22, 2019 08:47
@P0lip P0lip added the enhancement New feature or request label Aug 22, 2019
@P0lip P0lip marked this pull request as ready for review August 22, 2019 08:47
@P0lip P0lip requested review from billiegoose and marbemac August 22, 2019 08:47
@@ -0,0 +1,3 @@
module.exports = {
'plugins': ['@babel/plugin-transform-modules-commonjs'],
Copy link
Contributor

Choose a reason for hiding this comment

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

can you write a paragraph explaining why we need to add Babel to the ever-growing-list-of-compilers we need to support? Looks like it's only used by Jest?

Copy link
Contributor

Choose a reason for hiding this comment

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

hang on I just realized this isn't studio-internal 😆 um... I'm still curious though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because of of lodash-es. I suspect I should be able to instrument TS to transpile those files, though, so will give it a try.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see we replaced lodash - what is lodash-es doing for us?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rollup does not really like CJS modules, so this is the primary reason.
Lodash-es is mostly used in a worker code, so it does not really change much now.

P0lip and others added 2 commits August 28, 2019 20:23
Co-Authored-By: William Hilton <wmhilton@gmail.com>
@billiegoose
Copy link
Contributor

billiegoose commented Aug 28, 2019

@P0lip I made a few fixes

a very subtle off-by-one thingy.... the pre-rendered one didn't render the "half-row" at the bottom
2019-08-28 14 56 56

also don't use z-index it's evil and simple to avoid in this case by re-ordering the divs lol. 👍

oh and did you mean for it to have a computing prop that is never used? I removed that, hope that's OK.

@billiegoose
Copy link
Contributor

of course I broke the tests

@billiegoose
Copy link
Contributor

blegh. could not figure out the tests. fine, keep the flickering half-row at the bottom.

Copy link
Contributor

@billiegoose billiegoose left a comment

Choose a reason for hiding this comment

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

Most impressive.

@P0lip
Copy link
Contributor Author

P0lip commented Aug 29, 2019

a very subtle off-by-one thingy.... the pre-rendered one didn't render the "half-row" at the bottom

That's a feature? We want to have that cut to indicate the content can be scrolled

@P0lip P0lip merged commit 4bb6701 into master Aug 29, 2019
@P0lip P0lip deleted the feat/worker branch August 29, 2019 18:18
@stoplight-bot
Copy link
Collaborator

🎉 This PR is included in version 2.7.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

enhancement New feature or request released

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve overall perfomance Changing observed observable values outside actions is not allowed

5 participants