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

Document Component Guidelines #9933

Closed
w33ble opened this issue Jan 18, 2017 · 8 comments
Closed

Document Component Guidelines #9933

w33ble opened this issue Jan 18, 2017 · 8 comments
Labels

Comments

@w33ble
Copy link
Contributor

w33ble commented Jan 18, 2017

This was sparked by this comment on the UI cleanup issue.

We have started to write reusable components, using Angular directives and in some case React components. Many of us have some general ideas about what makes a good, reusable component, but it's important that we document them and create a reference for the whole team.

This issue is to track the creation of a component styleguide/reference. Where that lives and how it gets implemented is pretty much at the discretion of the person that does it.

@w33ble w33ble added the dev label Jan 18, 2017
@cjcenizal
Copy link
Contributor

cjcenizal commented Jan 18, 2017

This is borrowed from the email I sent out recently:

Examples of components

Angular components

Based on these examples, I can sort of see where we're going in terms of componentized directive design, because they all seem to exhibit these traits:

  1. They're relatively logic-free. Logic is provided via the attributes, in the form of callbacks and "providers" (think of a list or table directive accepting a "sort" function which provides the directive with sorting logic).
  2. They typically use ng-transclude to place child elements inside of an element in the template. From this perspective, they simply wrap children with specific markup.
  3. They're as small and simple as possible, sometimes consisting of a single element with a class or two on it.
  4. If a directive can enter various states (e.g. a button can enter binary states of disabled or enabled, or the multivariate states of "primary", "basic", and "danger"), then you can pass boolean flags or enum values to it via its attributes to set it to the desired state(s).

@w33ble
Copy link
Contributor Author

w33ble commented Jan 18, 2017

One of the things that your list of traits touches on, but doesn't explicitly say, is that their state should be pushed up as high as possible. That is, most reusable components should have no state at all, with all of their "state" coming in as attributes/props, and their consumer should be in charge of passing that in.

Also implied here, user interactions should be handled by passed in handlers, which trigger actions in the consumer, instead of mutating state directly.

@cjcenizal
Copy link
Contributor

cjcenizal commented Jan 18, 2017

I should also mention that I think using replace: true is a best practice, since markup structure and CSS can sometimes have some level of coupling (especially when flexbox is involved, since flexbox is all about the parent-child relationship).

@Bargs
Copy link
Contributor

Bargs commented Feb 9, 2017

An interesting issue came up during the event context review with @weltenwort. Because our version of Angular lacks one way binding it's difficult to separate local state (which is fine in small doses, IMO) from global state (which is evil).

The discussion came up in the context of a new size-picker component. size-picker accepts a count param, which it binds to an input element with ng-model. Under normal conditions, changes to the input element would propagate all the way up to whatever variable is two-way bound to count in the parent component, which is undesirable. @weltenwort came up with an interesting workaround using a getterSetter with ng-model. Instead of setting count directly, it would invoke a callback and rely on the parent component to update the state appropriately.

The problem is that if an error occurs in the callback the component will be left in an invalid state, with the input value set to the user entered value while the local $scope variable still holds the old value.

I suggested an alternative solution using a couple of watches to implement a poor man's one-way binding, but that wasn't without its own issues, it created a choppy user experience when @weltenwort tried it.

So, that's the (long winded) description of the problem. It would be cool if we could come up with a standard way to solve it. Does anyone else have ideas for other patterns that might work? Or what's keeping us from upgrading Angular?

@w33ble
Copy link
Contributor Author

w33ble commented Feb 21, 2017

I came across this problem recently as well. My solution was to copy the value in the controller's constructor, and use my copied value with ng-model.

app.directive('mySelect', function () {
  return {
    restrict: 'E',
    replace: true,
    template: `
      <select 
        ng-model="mySelect.value" 
        ng-change="mySelect.onValueChange(mySelect.value)"
      >
        <option value="1">1</option>
        <option value="2">2</option>
        <option value="3">3</option>
        <option value="4">4</option>
      </select>
    `,
    scope: {
      passedValue: '=value',
      onValueChange: '=',
    },
    controllerAs: 'mySelect',
    bindToController: true,
    controller: class mySelectController {
      constructor() {
        // avoid parent state mutation, since we have no one-way binding
        this.value = this.passedValue;
      }
    }
  };
});

It's not pretty, but it works, and it's quick and easy with primitive values. Objects and arrays would need to be deep cloned of course.

As for why we haven't updated Angular, I know someone looked at it a while back, but it didn't get completed. @spalger is this still on your radar?

@Bargs
Copy link
Contributor

Bargs commented Feb 22, 2017

@w33ble how do you propagate updates from passedValue to this.value if it changes after construction?

@w33ble
Copy link
Contributor Author

w33ble commented Feb 23, 2017

Good question. All I can say is that this worked correctly where I've done it already.

Maybe this needs a $watch on passedValue though? I honestly don't recall if you need a $watch on passed in values or not, I have a habit of thinking in React/Vue, and I don't use Angular for a long enough stretch remember 😞 .

@cjcenizal
Copy link
Contributor

Closing this as we’re moving to React and there’s a ton of material out there on best practices around component design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants