Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

fix(autocomplete): apply theming to input element #9698

Merged
merged 1 commit into from
Sep 4, 2018

Conversation

mckenzielong
Copy link
Contributor

Fixes #9447

@mckenzielong
Copy link
Contributor Author

mckenzielong commented Sep 27, 2016

@devversion similar to what we discussed in #9447 as option 1, but this isn't a full solution... I realized that the loading bar wouldn't be covered by this case (for example docs + accent yields proper coloured input, but incorrect styled loading bar)

We could filter the classes (md-warn and md-accent) as you mentioned and forward those to the input container and loading bar -- a similar solution could be applicable to md-input-container + md-select.

Edit: or I can add in the progress-linear css

Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

I've added two comments, those should make the review easier after.

@@ -218,6 +218,7 @@ function MdAutocomplete ($$mdSvgRegistry) {
// Retrieve the state of using a md-not-found template by using our attribute, which will
// be added to the element in the template function.
ctrl.hasNotFound = !!element.attr('md-has-not-found');
$mdTheming(element);
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to do this again? The controller already does register the theme.

https://github.com/angular/material/blob/master/src/components/autocomplete/js/autocompleteController.js#L70

background-color: '{{warn-color}}';
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

If we are doing something like that, then we should create a mixin for it, so we don't have to repeat it three times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@devversion not sure if you mean in autocomplete-theme.scss or if you mean on in input and linear progress that could be included?

Was hoping that the mixins used in md-tabs would have been a good starting point -- upon closer inspection they just repeated the same code in mixins:

@mixin md-tab-primary {
<- were you looking for something more like that?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I meant something like that 👍 This just removes the unnecessary repetitions and it's easier to maintain.

@devversion devversion added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: review This PR is waiting on review from the team labels Sep 30, 2016
@mckenzielong mckenzielong force-pushed the fix-autocomplete-input branch 2 times, most recently from 14a955e to dcb8aef Compare October 3, 2016 06:33
@devversion
Copy link
Member

devversion commented Oct 3, 2016

I just pulled the PR and tried it out. Seems like the progress bar did not get the correct color.

@devversion devversion self-assigned this Oct 3, 2016
@mckenzielong mckenzielong force-pushed the fix-autocomplete-input branch from dcb8aef to da3027b Compare October 4, 2016 01:50
@mckenzielong
Copy link
Contributor Author

@devversion Sorry, my bad. Fixed.

@devversion devversion removed the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Oct 10, 2016
@devversion
Copy link
Member

devversion commented Oct 10, 2016

I tried it out and it works 👍 Still not a fan of that solution, but it might be easiest for now.

@ThomasBurleson ThomasBurleson modified the milestones: 1.1.3, 1.2.0 Jan 1, 2017
@Splaktar Splaktar assigned Splaktar and unassigned devversion Aug 17, 2018
@Splaktar Splaktar modified the milestones: Future, 1.1.11 Aug 17, 2018
@Splaktar Splaktar added needs: manual testing This issue or PR needs to have some manual testing and verification done type: bug ui: theme P2: required Issues that must be fixed. ux: integration and removed needs: review This PR is waiting on review from the team labels Aug 17, 2018
@Splaktar Splaktar added the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Aug 17, 2018
@Splaktar
Copy link
Contributor

Splaktar commented Aug 17, 2018

@mckenzielong I'm sorry that this got stuck for so long. If you can rebase it, I'd be happy to try to get this merged in 1.1.11.

If you rebase, please update the commit to include the existing first line plus a blank line and then Fixes #9447.

If you aren't able to rebase it, I can try to take over this work and get it updated merged in.

@Splaktar Splaktar changed the title fix(mdAutocomplete): apply theming to input element fix(autocomplete): apply theming to input element Aug 17, 2018
@mckenzielong
Copy link
Contributor Author

@Splaktar i'll take a look 👍

@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Aug 20, 2018
@mckenzielong mckenzielong force-pushed the fix-autocomplete-input branch from da3027b to b5d2150 Compare August 20, 2018 15:23
@mckenzielong mckenzielong force-pushed the fix-autocomplete-input branch from b5d2150 to e5bb719 Compare August 20, 2018 15:24
@Splaktar Splaktar removed the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Aug 20, 2018
Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

LGTM. Verified with manual testing in light and dark modes. Thank you very much for this contribution! Sorry for the delay in getting it merged.

@Splaktar Splaktar added pr: merge ready This PR is ready for a caretaker to review and removed needs: manual testing This issue or PR needs to have some manual testing and verification done labels Aug 24, 2018
@Splaktar Splaktar added the pr: lgtm This PR has been approved by the reviewer label Aug 24, 2018
@Splaktar Splaktar assigned josephperrott and unassigned jelbourn Sep 4, 2018
@josephperrott josephperrott merged commit affe84b into angular:master Sep 4, 2018
nobitagit pushed a commit to nobitagit/material that referenced this pull request Sep 24, 2018
marosoft pushed a commit to marosoft/material that referenced this pull request Nov 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ P2: required Issues that must be fixed. pr: lgtm This PR has been approved by the reviewer pr: merge ready This PR is ready for a caretaker to review type: bug ui: theme ux: integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants