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.

Sign up for GitHub

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

Jump to bottom

Fix issues when using ReactElements in first-order types #285

Merged
merged 1 commit into from
Aug 18, 2019
Merged

Fix issues when using ReactElements in first-order types #285

merged 1 commit into from
Aug 18, 2019

Conversation

threeseed
Copy link
Contributor

No description provided.

@threeseed threeseed mentioned this pull request Jul 21, 2019
Closed
@shadaj
Copy link
Owner

shadaj commented Jul 21, 2019

Looks like the build is failing due to incompatibility with 2.13? Will take a detailed look ASAP.

shadaj
shadaj requested changes Jul 22, 2019
View reviewed changes
Copy link
Owner

@shadaj shadaj left a comment

Choose a reason for hiding this comment

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

First pass. I'm not super sure if we should be dealing with so many types right now. The main pain points are when using sequences and options, so maybe we should consider just providing conversions for those to start.

core/build.sbt Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
core/src/main/scala/slinky/core/facade/React.scala Outdated Show resolved Hide resolved
core/src/main/scala/slinky/core/facade/React.scala Outdated Show resolved Hide resolved
core/src/main/scala/slinky/core/package.scala Outdated Show resolved Hide resolved
tests/src/test/scala/slinky/core/annotations/ReactAnnotatedComponentTest.scala Outdated Show resolved Hide resolved
@shadaj
Copy link
Owner

shadaj commented Aug 17, 2019

Could you merge with master? I recently switched the CI to GitHub Actions, looks like it doesn't run unless the configuration is present in your branch.

@shadaj
Copy link
Owner

shadaj commented Aug 17, 2019

Ah, the build docs failure is my mistake. I'll get that fixed in a bit.

@threeseed
Copy link
Contributor Author

I have cleaned up the Git history so it aligns with existing style.

shadaj
shadaj requested changes Aug 17, 2019
View reviewed changes
Copy link
Owner

@shadaj shadaj left a comment

Choose a reason for hiding this comment

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

Just a couple more changes, but almost there! Will need a rebase again to fix the bundlesize CI failures.

tests/src/test/scala/slinky/core/annotations/ReactAnnotatedComponentTest.scala Show resolved Hide resolved
tests/src/test/scala/slinky/core/annotations/ReactAnnotatedComponentTest.scala Outdated Show resolved Hide resolved
tests/src/test/scala/slinky/core/annotations/ReactAnnotatedComponentTest.scala Outdated Show resolved Hide resolved
docs/public/docs/installation.md Outdated Show resolved Hide resolved
tests/build.sbt Outdated Show resolved Hide resolved
core/src/main/scala/slinky/core/facade/React.scala Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
shadaj
shadaj approved these changes Aug 18, 2019
View reviewed changes
Copy link
Owner

@shadaj shadaj left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for this awesome improvement!

@shadaj shadaj merged commit cc9d534 into shadaj:master Aug 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shadaj shadaj shadaj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@threeseed @shadaj @harana-bot