-
Notifications
You must be signed in to change notification settings - Fork 5k
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
feat(base): Create v2 base components. #4600
Conversation
#### ES2015+ | ||
|
||
```javascript | ||
import MDLBase, {MDLBaseFoundation} from 'mdl-base'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about the alternative nomenclature of:
MDLBase => MDLComponent
MDLBaseFoundation => MDLFoundation
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was that having Base
there is more explicit, but I could go either way. MDL{Component,Foundation}
is less verbose which is probably better. I'll change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would lean towards MDL{Component,Foundation}
unless we are planning to use them for something else. Although I understand there's a bit of a subtle discrepancy with the package/class naming (classes in base wouldn't necessarily be prefixed by base, whereas generally for components they would be prefixed by the component name). Though, I like the idea of keeping the focus on the main keywords {Component, Foundation} throughout the docs. (base isn't really a concept in itself)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Updated in code and I'll update the README as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only caveat here is that we wouldn't be able to have a MDL{Component, Foundation} interface. MDLI{Component, Foundation} ? Just kidding. So you wouldn't be able to build a foundation/component without inheriting from the abstract classes.
2d2b937
to
288778f
Compare
| --- | --- | | ||
| cssClasses | returns an object where each key identifies a css class that some code will rely on. | | ||
| strings | returns an object where each key identifies a string constant, e.g. `ARIA_ROLE` | | ||
| numbers | returns an object where each key identifies a numeric constant, e.g. `TRANSITION_DELAY_MS` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would we want to overwrite the number constants? Or is it just for consistency w.r.t. other constants?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consistency, and not having magic numbers in code is a good convention. This was moved over from the initial MDL v2 stuff.
LGTM, no other comments. |
d8f8061
to
d163011
Compare
- Change `mdl-base-component` -> `mdl-base` - Add description and correct license to `mdl-base` - Implement base foundation and component classes - Deprecate MDLBaseAdapter; remove from index exports - Update all build and dependency targets - (extra) Fix false positive in mdl-auto-init test - (extra) Update quotes eslint rule to avoid escaping - (extra) Fix failing ripple tests - (extra) Allow TravisCI to run all PRs via saucelabs Resolves #4579 [Delivers #126819203]
d163011
to
d27a511
Compare
The TravisCI tests are failing due to flaky Sauce Labs tests. I've opened up a ticket with Sauce Labs and will be investigating the issue more, but for now I'm going to merge this in as to unblock the rest of our work. I manually ran tests in all browsers and verified that they were successful. Hopefully we can get to the bottom of this issue quickly, otherwise we may have to investigate alternative cross-browser testing methods. |
mdl-base-component
->mdl-base
mdl-base
Resolves #4579
[Delivers #126819203]