Skip to content
This repository has been archived by the owner on Jul 30, 2018. It is now read-only.

Update README for Themed changes #735

Merged
merged 4 commits into from
Nov 1, 2017
Merged

Conversation

agubler
Copy link
Member

@agubler agubler commented Oct 31, 2017

Type: feature

The following has been addressed in the PR:

  • There is a related issue
  • All code matches the style guide
  • Unit or Functional tests are included in the PR

Description:

README updates for the Themed changes

@agubler agubler requested review from dylans, tomdye and kitsonk October 31, 2017 16:44
dylans
dylans previously requested changes Oct 31, 2017
Copy link
Member

@dylans dylans left a comment

Choose a reason for hiding this comment

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

minor nits, otherwise looks good to me.

README.md Outdated
@@ -321,9 +321,9 @@ export const classNameTwo: string;

**Important:** at runtime a JavaScript file is required to provide the processed CSS class names.

The `ThemeableMixin` provides a method available on the instance `this.classes()` that consumes string class names and converts them into the expected class object when a widget is rendered. Classes that are passed to `this.classes()` are made available to be themed, as described in the [applying a theme](#applying-a-theme) section.
The `ThemedMixin` provides a method available on the instance `this.theme()` that consumes string class names and returns the themes equivalent class names.
Copy link
Member

Choose a reason for hiding this comment

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

themes equivalent class names -> theme's equivalent class names

README.md Outdated
}
}
```

If an array is passed to `this.theme` then an array will be returned for example, `this.theme([ css.root, css.other ])` will return an array containing the theme's class names `[ 'themedRoot', 'themedOther' ]`.
Copy link
Member

Choose a reason for hiding this comment

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

be returned for example, -> be returned. For example,

@agubler
Copy link
Member Author

agubler commented Oct 31, 2017

@dylans updated

Copy link
Member

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

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

Some comments about clarity...

@@ -334,11 +334,13 @@ import { ThemeableMixin, theme } from '@dojo/widget-core/mixins/Themeable';
@theme(css)
export default class MyWidget extends ThemeableMixin(WidgetBase) {
protected render() {
return v('div', { classes: this.classes(css.root).fixed(css.rootFixed) });
return v('div', { classes: [ this.theme(css.root), css.rootFixed ] });
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this supposed to be:

-return v('div', { classes: [ this.theme(css.root), css.rootFixed ] })
+return v('div', { classes: [ ...this.theme(css.root), css.rootFixed ] })

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind... I see below about it... it does feel a bit confusing without an example maybe actually showing a code example after the return array and spreading it would be useful. Also, IIRC from the changes, this.theme() only accepts a single argument? If that is correct it might be better to be a bit more specific above when we say this.theme() that consumes string class names and returns the theme's equivalent class names. to something like this.theme() takes a single argument that is either a string class name or an array of string class names and returns the theme's equivalent class names as either a single string or array of strings?

@agubler agubler dismissed dylans’s stale review November 1, 2017 17:49

comments addressed

@agubler agubler merged commit 39248fb into dojo:master Nov 1, 2017
@dylans dylans added this to the beta.4 milestone Nov 1, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants