Skip to content

Refactoring to streams#2

Open
johnlindquist wants to merge 2 commits intoDeborahK:masterfrom
johnlindquist:master
Open

Refactoring to streams#2
johnlindquist wants to merge 2 commits intoDeborahK:masterfrom
johnlindquist:master

Conversation

@johnlindquist
Copy link
Copy Markdown

So here's a bit more of what I had in mind:

  1. Refactored all the validation messages into streams
  2. Removed all ngClass in favor of the .ng-invalid class
  3. Refactored the NumberValidator to output errors messages on min/max/NaN
  4. Created a help-block component (I prefer "dumb component" with @inputs for handling streams)

The main benefit here is now all the message streams could combine/compare with one another for validation across multiple fields. You could also create additional streams off of the message streams to invoke other logic/services based on complex scenarios.

This is meant more of a "here's my thoughts" than a pull request.

Cheers,
John

Removing ngClass in favor of ng-invalid
Refactoring movie to streams
Refactoring NumberValidator to returns min/max/NaN errors
Refactoring the help component
@DeborahK
Copy link
Copy Markdown
Owner

This makes more sense.
One question, what is the benefit of using the async pipe on a hard-coded property value (pageTitle)?

@johnlindquist
Copy link
Copy Markdown
Author

From my point of view, pageTitle has a default and a streamed value from the movie service (since all http.get requests are streams). So it makes sense to handle the result as a stream as well. Maybe the pageTitle stream would read better with .startWith('Add Movie') then pushing subsequent values as they arrive.

The pageTitle as a stream could now display errors, timers (e.g., "you will be logged out in 3... 2..."), and more importantly live-updated data. If you had multiple users editing the form, you could fairly trivially "push" the new title in each time a different user and/or server changed it.

Granted, this is usually overkill for demos/samples, but the benefits payoff as you start adding more complex features.

@DeborahK
Copy link
Copy Markdown
Owner

Cool info! 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.

2 participants