-
Notifications
You must be signed in to change notification settings - Fork 21
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
Avoid timeout and others #1
Comments
@julianobrasil Thank you for this excellent feedback! About the setTimeout(), I have studied this and found that it is not necessary after all. So it is now removed. I was curious how the Material team avoids setTimeout(), so I searched the code and found that it's actually used somewhat widely. I see the desire to avoid it, but it is not always avoidable, apparently. About testing, I have been thinking about how to effectively auto-test this code. Right now it is incidentally tested because we use it in various "real work" downstream. But I would like to have a way to test it automatically right here in this repo. About the notion of a PR - I have constructed this so far to live in "user space" - nowhere near being eligible as a PR. It is not a fork of Angular Material; it does not add any new component as a "peer" of the existing components; etc. Rather, I mentioned originally the idea of a PR for two reasons:
About the |
Yeah... one thing is what you want... other thing is what is possible to do. 😄
Oh, now it makes sense. The code seems to be very very organized, but to live in this "user space".
Your code express very well the aplicability of this pattern to this kind of component I have a lot of autocompletes in some of my projects. All of them making undesired use of As it goes to production environment in the university where I work (I'm a professor in Electrical Engineering course, who accidentaly fell in love with software development), I've imposed to myself to just use "long term" pure angular libraries. Right now I'm using:
As I refactor my autocompletes, if you wish, I can try out your |
@julianobrasil I would be thrilled if you would try my component in an application and send me your feedback about the functionality and developer ergonomics. We have several people using it here at work already, but they have the advantage of being able to talk with the person who wrote the code all the time (me), so the feedback from them probably doesn't fully capture whatever difficulties there are in using the code. Our strategy here is very similar to yours, we avoid mixing in ad hoc non-angular JavaScript code is much as we possibly can, and similarly use as much as possible, the official Angular libraries. No professors here, but I have personally taught several hundred people Angular, and our group here has taught thousands. ( https://angularbootcamp.com/ ) |
That's awesome. Let this issue open so I can use it to give you a feedback in a few days. |
(Anyone with any similar feedback or questions, please feel free to open a new issue!) |
Is it possible to avoid the
setTimeout
? As you plan a future PR in material 2 repo, the team avoids usingsetTimeout
's.https://github.com/OasisDigital/angular-material-obs-autocomplete/blob/b5e1a5fdd43b7a8e0391760e84acb2b26c97fc52/src/app/obs-autocomplete/obs-autocomplete-base.ts#L42-L49
You could take a look in
MatAutocompleteTrigger.panelClosingActions
as it's shown over here: angular/components#3334 (comment)Also, there are recommendations about coding in Material repo: https://github.com/angular/material2/blob/master/CODING_STANDARDS.md
I've noticed the code style is a very important aspect among the guys developing Material. Tests are mandatory. You'll have to write them.
And, above all that... ask them whether you should be doing it before spending time preparing your PR. Not all features in Material Repo are opened for direct contribution. This kind of feature seems to me it's something that should be part of the project core. So there's a guideline that any core feature must be exhaustively discussed (internally to Google). Force selection is in this step (you can be sure that a topic with more than 40 comments is not being ignored by the project leader, Jeremy).
Rejecting PR's is not so frequent, but if your work is out of sync with their timeline, it can be frozen for months or even years awaiting for a merging action (and it's possible that you'll have to rebase it then).
Dispite of all this bureaucracy thing, I think your component is very nice, and your examples are really great (but the
setTimeout
thing... I don't like the idea of counting on timing to take actions - even in this case, where aparently you are just pushing the subscription to the end of the line in the event loop).[edited]: shouldn't it be
this.selectedValueSub = this.selectedValue.pipe(...
The text was updated successfully, but these errors were encountered: