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(component): allow delegatesFocus on the component decorator #1190

Closed
wants to merge 11 commits into from
Closed

feat(component): allow delegatesFocus on the component decorator #1190

wants to merge 11 commits into from

Conversation

MrAntix
Copy link
Contributor

@MrAntix MrAntix commented Oct 30, 2018

ref: #1008

@adamdbradley
Copy link
Contributor

I like this, but wonder if we should instead have the options object passed in, rather than making a new option to the component decorator.

@manucorporat
Copy link
Contributor

Agreed with Adam, i think it should be an object, probably allowing the shadow property to be both boolean or ShadowDomOptions:

 @Component({
     shadow: {
         delegateFocus: true
     }
  })

@adamdbradley
Copy link
Contributor

@MrAntix would you be able to make the changes so shadow could either be a boolean, or an object? If it's an object we merge it with { mode: 'open' }. And if it's a boolean, the object should be { mode: 'open' }.

@MrAntix
Copy link
Contributor Author

MrAntix commented Oct 30, 2018

hows that look?

@adamdbradley
Copy link
Contributor

Looking great. One thing is that in most cases, shadow: true will be the case, and the options would be used less often. So I think the net result of less could we be to not always build an options object, where every component would set {mode:'open'}, but rather have the run do the merge instead.

Copy link
Contributor

@manucorporat manucorporat left a comment

Choose a reason for hiding this comment

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

I added some comments, I think the runtime should provide the default value, let's not make the metadata bigger for the common case.

src/core/host-snapshot.ts Outdated Show resolved Hide resolved
src/declarations/component.ts Outdated Show resolved Hide resolved
make delegatesFocus available
```@Component({ ... shadow?: boolean | ShadowDomOptions  })```
@MrAntix
Copy link
Contributor Author

MrAntix commented Oct 31, 2018

all done, do you need the commits squashed?

@manucorporat
Copy link
Contributor

@MrAntix don't worry about it! we can squash them before merge

@adamdbradley, @jthoms1 the PR looks good to me!

@tehdb
Copy link

tehdb commented Jan 4, 2019

please merge and publish 🙏

@zoofadoofa
Copy link

This is not really an edge case, anything that is slotting in content into shareable components will need delegatesFocus
See Demo

@MrAntix
Copy link
Contributor Author

MrAntix commented May 5, 2019

Could this be released? I won't have capacity to upgrade to v1 as the breaking changes are too big and my project is going live with 0.18.x

@romulocintra
Copy link
Contributor

Any news ;) any help needed ?

@driskull
Copy link

This would be nice to have

@adamdbradley
Copy link
Contributor

adamdbradley commented Dec 18, 2019

Closing in favor of ad94fd2

@MrAntix
Copy link
Contributor Author

MrAntix commented Dec 18, 2019

Thanks

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.

7 participants