Skip to content

Conversation

@IvayloG
Copy link
Contributor

@IvayloG IvayloG commented Aug 27, 2020

Closes #6751

Additional information (check all that apply):

  • Bug fix
  • New functionality
  • Documentation
  • Demos
  • CI/CD

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code (test guidelines)
  • This PR includes API docs for newly added methods/properties (api docs guidelines)
  • This PR includes feature/README.MD updates for the feature docs
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes ng update migrations for the breaking changes (migrations guidelines)
  • This PR includes behavioral changes and the feature specification has been updated with them

@IvayloG IvayloG requested a review from ViktorSlavov August 28, 2020 06:41
*/
@Input()
public type = 'line';
public set type(value: IgxInputGroupType) {
Copy link
Contributor

@ViktorSlavov ViktorSlavov Aug 31, 2020

Choose a reason for hiding this comment

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

This implementation works fine, but I feel like it should be the same as in the IgxInputGroup component. If we expose the input to be templateable or if (for some reason) a user passes items that have and input in them, those should behave consistently w/ the input group from the view template. So the select component should get the InputGroupToken injected in it's constructor (optional) and type should return this._type || this._inputGroupToken || 'line'. As we are passing any specified type to the input in the template, there should be no inconsistencies when it is provided.
@IvayloG , @damyanpetev , thoughts?

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 might be an issue, but the input group child may one day not be static and interfere with this setter. So I'd still go that route anyway.

ViktorSlavov
ViktorSlavov previously approved these changes Aug 31, 2020
*/
@Input()
public type = 'line';
public set type(value: IgxInputGroupType) {
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 might be an issue, but the input group child may one day not be static and interfere with this setter. So I'd still go that route anyway.

* Defines the InputGroupType DI token.
*/
// Should this go trough Interface https://angular.io/api/core/InjectionToken
export const InputGroupToken = new InjectionToken<IgxInputGroupType>('InputGroupType');
Copy link
Member

Choose a reason for hiding this comment

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

I'd take the more fitting token name of IGX_INPUT_GROUP_TYPE

@damyanpetev damyanpetev added 💥 status: in-test PRs currently being tested and removed ❌ do not merge ❌ status: awaiting-test PRs awaiting manual verification labels Sep 3, 2020
/**
* An @Input property that sets how the select will be styled.
* The allowed values are `line`, `box` and `border`. The default is `line`.
* The allowed values are `line`, `box` and `border`. The input-group default is `line`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit-pick, but this change isn't really needed

@PlamenaMiteva PlamenaMiteva self-assigned this Sep 4, 2020
@PlamenaMiteva PlamenaMiteva added ✅ status: verified Applies to PRs that have passed manual verification and removed 💥 status: in-test PRs currently being tested labels Sep 4, 2020
@damyanpetev damyanpetev merged commit ab84729 into master Sep 4, 2020
@damyanpetev damyanpetev deleted the iganchev/input-group-token-master branch September 4, 2020 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature input-group version: 10.2.x ✅ status: verified Applies to PRs that have passed manual verification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expose way to set the Input Group type globally since input components depend on it to apply themes correctly.

5 participants