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

refactor(): value types for directives expecting simple string or string combinations #902

Closed
wants to merge 4 commits into from
Closed

Conversation

vmasek
Copy link

@vmasek vmasek commented Dec 4, 2018

This will enable input value completion in IDEs when using fxLayout, fxFlexAlign, fxShowHide and fxLayoutAlign directives

I used supported values from directives API docs in wiki
and also included some that are not mentioned there

  • flex- prefixes for start and end
  • baseline for LayoutAlign which was not mentioned in wiki fxLayoutAlign-API

Tested in WebStorm with Angular Language service enabled.

fxflexalign

fxlayout

fxshow

fxlayoutalign

fxlayoutalign-start

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@googlebot googlebot added the cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ label Dec 4, 2018
@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ labels Dec 4, 2018
@CaerusKaru
Copy link
Member

In principle, I have no issue with this. It wouldn't affect the runtime build size because all of this is typed metadata, and it's great for developer experience.

Unfortunately, with #900, we're getting rid of individual inputs in favor of making the custom breakpoints story easier for developers, which I think is more of a benefit of this change.

If there is a way to get this in without changing the @Input type I would be more than happy to discuss/review.

@CaerusKaru
Copy link
Member

I've also updated the Wiki for fxLayoutAlign to reflect the baseline input. Thanks for catching that!

@vmasek
Copy link
Author

vmasek commented Dec 6, 2018

@CaerusKaru I see, is it because of
export class CustomShowHideDirective extends ShowHideDirective ... constructor?
https://github.com/angular/flex-layout/wiki/Breakpoints

It's a pitty that the inputs are removed, it now lacks a type safety and using inputs in metadata decorator is a bad practice according to angular style guide.

I'd like to have a closer look on the possibilities, but I don't fully understand the motivation and changes in mentioned PR. Would you have a time for some intro?

@CaerusKaru
Copy link
Member

@vmasek It's not because of the constructor, we're actually refactoring so that you don't need to include one in a custom breakpoint directive as part of #900.

I have two points about type safety and the style guide. For type safety, inputs generally don't require them, especially in our library where most directives accept any type of value and are coerced to the correct type at runtime. We have documentation that provides the options for our users, and while it would definitely be easier to have an inline reference, maybe a better place for that would be the package README (or the module README?).

As for the style guide, it is a general best practice guide for end users, not library authors. Angular Material, for instance, violates many of the style guide core principles, most notably by using the host metadata parameter on practically every directive. It makes it more convenient and maintainable for a more complex codebase, at least in some people's opinions including my own.

If we were to not follow this, and revert back to using Input decorators, we would need to declare n number of Inputs based on each component. If we simply declare an array, we don't need to declare any Inputs, and they all get funneled into central processing by ngOnChanges. This gives us the flexibility of saying ngOnChanges and the inputs array corresponds 1:1 with mediaQuery processing, and all Inputs are strictly for additional functionality (like fxShrink and fxGrow).

This is the motivation behind #900, to reduce complexity and overhead introduced by having a ton of Inputs for each directive, as opposed to centralizing the input functionality to one place. Please head over to #903 for some more explanation and discussion on the issue.

@vmasek
Copy link
Author

vmasek commented Dec 6, 2018

Thank you, in that case, I am closing this PR, if I manage to come up with some other solution for this, I will discuss it in new PR/Issue.

@vmasek vmasek closed this Dec 6, 2018
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants