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(slots): functional component text slots #733

Merged
merged 1 commit into from
Jun 18, 2018
Merged

fix(slots): functional component text slots #733

merged 1 commit into from
Jun 18, 2018

Conversation

TheAlexLichter
Copy link
Contributor

Resolves #732

jacekkarczmarczyk
jacekkarczmarczyk reviewed Jun 18, 2018
View reviewed changes
packages/server-test-utils/dist/vue-server-test-utils.js Outdated
@@ -19,14 +19,12 @@ function createVNodesForSlot (
slotValue,
name
) {
if (typeof slotValue === 'string' &&
!startsWithTag(slotValue)) {
if (typeof slotValue === 'string' && !startsWithTag(slotValue)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes are not related to the issue, files are automatically updated on yarn test or something, but they shouldn't be included in the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops! Thanks ☺️

packages/create-instance/create-functional-component.js Outdated
if (Array.isArray(slots.default)) {
return slots.default.map(h)
}
function startsWithTag (str) {
Copy link
Contributor

Choose a reason for hiding this comment

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

startsWithTag is also defined in add-slots.js, I guess it could be extracted to some helper

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right! Actually, we could use the add-slots files as well.

Let me refactor it 👍

packages/test-utils/dist/vue-test-utils.js Outdated
@@ -17,7 +17,10 @@ function warn (msg) {

var camelizeRE = /-(\w)/g;
var camelize = function (str) {
var camelizedStr = str.replace(camelizeRE, function (_, c) { return c ? c.toUpperCase() : ''; });
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes are not related to the issue, files are automatically updated on yarn test or something, but they shouldn't be included in the PR

@TheAlexLichter TheAlexLichter force-pushed the fix-functional-text-slots branch from 6893811 to 31a3d0b Compare June 18, 2018 08:31
@TheAlexLichter
Copy link
Contributor Author

TheAlexLichter commented Jun 18, 2018 •
edited
Loading

@jacekkarczmarczyk I've refactored it. But I've encountered a strange behavior:

Currently, the implementation uses the createFunctionalSlotVNodes function. This is just a "redirect" (see here):

export function createFunctionalSlotVNodes (h, slots = {}) {
  return createSlotVNodes(h, slots)
}

However, if I replace all calls with the createSlotVNodes function, karma throws several errors.

Can't explain the behavior to me:man_shrugging:
Do you have any idea?

EDIT: And flow is failing because of types

@jacekkarczmarczyk
Copy link
Contributor

You've overwritten bc736fb ...

@TheAlexLichter
Copy link
Contributor Author

TheAlexLichter commented Jun 18, 2018 •
edited
Loading

@jacekkarczmarczyk Dang! 🙄 Still too early for me 🙈

Should look better now...

@TheAlexLichter TheAlexLichter force-pushed the fix-functional-text-slots branch 2 times, most recently from e1d576b to ca7f057 Compare June 18, 2018 08:51
@TheAlexLichter
Copy link
Contributor Author

Okay, one more try. I think I got it now. Test running locally and flow doesn't complain anymore 😋

eddyerburgh
eddyerburgh reviewed Jun 18, 2018
View reviewed changes
packages/create-instance/add-slots.js Outdated
}, [])
}

export function createFunctionalSlotVNodes (
Copy link
Member

@eddyerburgh eddyerburgh Jun 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

No need to add this function, we can just use createSlotVNodes

eddyerburgh
eddyerburgh reviewed Jun 18, 2018
View reviewed changes
packages/create-instance/add-slots.js Outdated
@@ -6,7 +6,7 @@ function startsWithTag (str) {
return str && str.trim()[0] === '<'
}

function createVNodesForSlot (
export function createVNodesForSlot (
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to export

eddyerburgh
eddyerburgh reviewed Jun 18, 2018
View reviewed changes
packages/create-instance/create-functional-component.js Outdated
})
return children
}
import { createFunctionalSlotVNodes } from './add-slots'
Copy link
Member

Choose a reason for hiding this comment

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

Can you use createSlotVNodes instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this, but it gave me some strange errors.

See #733 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: Looks like this had sth. to do with wrong types. It works fine right now 🤷‍♂️

eddyerburgh
eddyerburgh requested changes Jun 18, 2018
View reviewed changes
Copy link
Member

@eddyerburgh eddyerburgh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the PR, looks good although I think we can use the existing createSlotVnodes function instead of the new function

edit: I've just seen your comments about karma. Can you post the errors?

@TheAlexLichter TheAlexLichter force-pushed the fix-functional-text-slots branch from ca7f057 to 0a133fd Compare June 18, 2018 09:28
@TheAlexLichter
Copy link
Contributor Author

@eddyerburgh I think those were related to flow issues as everything works fine now ☺️

I've updated the PR. Hope the tests will pass like they do locally 👍

eddyerburgh
eddyerburgh approved these changes Jun 18, 2018
View reviewed changes
Copy link
Member

@eddyerburgh eddyerburgh left a comment

Choose a reason for hiding this comment

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

Great, thanks 👍

A PR that removes more code than it adds, AND fixes a bug is always good!

@eddyerburgh eddyerburgh merged commit daa56aa into vuejs:dev Jun 18, 2018
@TheAlexLichter TheAlexLichter deleted the fix-functional-text-slots branch June 18, 2018 09:43
@TheAlexLichter
Copy link
Contributor Author

@eddyerburgh You are welcome ☺️

Cannot agree more on that! 🙊

@TheAlexLichter
Copy link
Contributor Author

TheAlexLichter commented Jun 18, 2018 •
edited
Loading

@eddyerburgh Do you know when the change will be released? :)

@eddyerburgh
Copy link
Member

eddyerburgh commented Jun 18, 2018 via email

This was referenced Jun 18, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacekkarczmarczyk jacekkarczmarczyk jacekkarczmarczyk left review comments

@eddyerburgh eddyerburgh eddyerburgh 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
@TheAlexLichter @jacekkarczmarczyk @eddyerburgh