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

Use modular package structure for all UI web components #14

Closed
ccmitchellusa opened this issue Oct 1, 2013 · 17 comments
Closed

Use modular package structure for all UI web components #14

ccmitchellusa opened this issue Oct 1, 2013 · 17 comments
Assignees

Comments

@ccmitchellusa
Copy link
Member

When dui common package is separated from the concrete widgets, I'd recommend that we refactor the concrete widgets so that there is a folder/package per web component that is self-contained in terms of templates, html, css, tests, docs rather than all widget code in a single directory. This will help enforce modularity of the widgets. A similar approach is being used on the polymer-ui-elements repository https://github.com/Polymer/polymer-ui-elements.

@wkeese
Copy link
Member

wkeese commented Oct 2, 2013

Hmm, it's an interesting idea but what would each folder look like? I can see it would have the template (for widgets with templates), but the main thing would be the CSS, and that leads to the question of the source vs. the generated CSS. If both were moved the structure might look something like:

Button/
     Button.js
     Button.html
     themes/
         Button.less
         ios/
             Button.css
         holodark/
             Button.css
         ios/
             Button.css

The other question is whether the JS file is in the folder or not; I guess it is?

@cjolif
Copy link
Contributor

cjolif commented Oct 2, 2013

And that means:

require(["dui/button/Button", "dui/list/List"], function(Button, List){});

I'm not sure our users will particularly appreciate the verbosity and the duplication of the name of widget vs:

require(["dui/Button", "dui/List"], function(Button, List){});

I think polymer does not have the problem because they are "only" markup, not programmatic. I think this should be taken into account because modularity is great but we need to not make things too cumbersome to use (especially in 2.0 the user will anyway not see/have to deal with the CSS as the CSS plugin will load them automatically).

@ccmitchellusa
Copy link
Member Author

Re naming...
dui/Button/element <- the element.js controller logic for the button element.

Re theming, the Button/element.css can have the base/generic styling (if any for a widget, like dijit.css rules but specific to this component).

theme.css can augment/override these rules separately.
Themes should be in their own packages of web resources (css, images, web fonts for icons, etc.)

The directory names within dui for components are essentially self-contained "sub packages" with their own tests, docs, etc. removing a subpacakge directory should remove the corresponding docs, tests, styles, templates, etc. associated with that corresponding component.

@wkeese
Copy link
Member

wkeese commented Oct 8, 2013

dui/Button/element <- the element.js controller logic for the button element.

You mean have two JS files, Button.js and Button/element.js ? It sounds like overkill and will slow down anyone not running on a build. I know that it's politically correct to talk about separating controller logic from presentation logic, but its meaningless for something as simple as Button. For more complex widgets it's unclear.

Re theming, the Button/element.css can have the base/generic styling
... Themes should be in their own packages of web resources (css, images, web fonts for icons, etc.)

I really don't want to have a "generic styling" file. It's easier just to have a Button.less file (or Button.styl file) that each theme uses to generate its own Button.css.

It would be possible to put the Button.less file outside of the themes directory. Not sure if that's an improvement or not.

@ccmitchellusa
Copy link
Member Author

no, dui/Button/element is the mid for element.js in the directory (in the directory dui/Button). controller just means behavior/logic that responds to the ui events in this case, nothing more complex that that.

@ccmitchellusa
Copy link
Member Author

that's what i mean, put the Button.styl or .less in the component's directory.
In other words:

dui/Button/           <== the directory for the Button web component's sub-package
 element.html   <== The web component's template
 element.js       <== The web components's behavior/logic/controller/script
 element.styl | .less     <== The base styling for the button web component (optional if the component is trivial).
 images/          <== Optional if not needed
 nls/                <== Optional if localization needed for this web component 
 …                  <== Possibly other things like docs/ , tests/ specific to this web component.
myCoolTheme/     <== Package for my cool custom theme.
  Button.less      <== Override dui/Button.style/less to generate Button.css, which gets aggregated into myCoolTheme.css via @import processing.
require(["dui/Button/element", "dui/List/element"], function(Button, List){});

@wkeese
Copy link
Member

wkeese commented Oct 9, 2013

OK, thanks for the picture. It's an interesting idea. Seems like the only troublesome part (as Christophe mentioned) is that its inconvenient for everyone including that element into their page because they need to type

require(["dui/Button/element", ...

instead of just

require(["dui/Button", ...

Same issue for anyone subclassing Button.

That's why it might be better to move Button/element.js --> Button.js, i.e.:

dui/Button.js   <== The web components's behavior/logic/controller/script
dui/Button/           <== the directory for the Button web component's sub-package
 element.html   <== The web component's template  
 element.styl | .less     <== The base styling for the button web component (optional if the component is trivial).
 images/          <== Optional if not needed
 nls/                <== Optional if localization needed for this web component 
 …                  <== Possibly other things like docs/ , tests/ specific to this web component.

@cjolif
Copy link
Contributor

cjolif commented Oct 9, 2013

I do prefer Bill's alternative. This allows to keep a consistent view of the simple UI widgets instead of putting additional burden on the users to find them in subpackages.

I think this is the good level of granularity because I don't believe a second someone will just use our "Button" so it does not need to be fully separated in a different package.

Obviously that's different for higher level widgets like charting which should get their own package outside of dui. We can also imagine some higher level widgets in dui getting their own subpackage (Tree, List...) but simple widgets like Button or Togglebutton should really be in same directory with all their resources (css, templates...) in a sub-directories like Bill mentioned.

@cjolif
Copy link
Contributor

cjolif commented Oct 9, 2013

Maybe using a lower-case (i.e. button instead of Button) for the sub-directory would look nicer. But then I agree this is not fully consistent with the class name.

@ccmitchellusa
Copy link
Member Author

Maybe configuration module aliases on the AMD would help solve the longer name problem here, rather than having to move the Button.js out of it's package directory, apps can use use loader config to map the mid's to whatever, while dui modules would use the slightly longer form.

Another thought to limit typing is just use "el.js" rather than element for the element.js name, making the default mid dui/Button/el

eg. can even remove the dui namespace altogether, making the web component appear as it it was from it's own standalone package:

require(["Button"],…

@pruzand
Copy link
Member

pruzand commented Oct 9, 2013

I do like Bill's proposal too: it keeps the packaging clean, structured, readable, and easy to use for a developer.

Chris, re. the config module alias... Remember how the AMD upgrade has been painful (and still is) for a lot of users, especially due to the complexity of the loader configuration. To me, it is just unrealistic to ask the dui users to tweak their loader configuration (most of them don't know about it anyways) to define aliases on our classes, because our packaging is too complex to use. In fact, if we have to explain that the recommended way to use our lib is to define loader aliases, then it is the proof there's a problem in our packaging.
To summarize, I think Bill's proposal is a good compromise.

@ccmitchellusa
Copy link
Member Author

Yes, I agree that the loader config aliasing will be more confusing to users.

As long as dui (or whatever we end up calling the package containing the web component sub packages once they've been separate from the abstract framework package) is cohesive in terms of use case and kept small Bill's approach should work fine. Because we'll have a bunch of .js web component modules in the top level directory, it could get to the point where we have too much, forcing us to split subsets of the components or individual more complex components out into their own standalone packages if dui begins to get too heavy.

Has there been any thoughts as to what is the criteria for widgets to be included within dui/ components package rather just delivering them as standalone packages (eg. gridx)? Should we open a separate issue to discuss this under?

@cjolif
Copy link
Contributor

cjolif commented Nov 6, 2013

Looks like we have decision so I guess we should first apply it to the few widgets we have right now and keep doing it for future widgets?

@wkeese
Copy link
Member

wkeese commented Nov 6, 2013

You'd need to change themes/load.js and themes/utils/compile.js at the same time though; neither is expecting less files and CSS files spread out into different directories like that.

I'm not sure what we do with the various themes' variables.less files. Do they stay where they are?

The load.js change may be particularly tricky, considering there are files like common/common.less and common/dnd.less that don't correspond to any widget, but are loaded via load!common.

@cjolif
Copy link
Contributor

cjolif commented Nov 7, 2013

What about making that explicit and ask the component developer to put the right path when requiring the CSS?

The component developper would have to do

require(["dui/css!./Button/button.css", "dui/css!/common/othercss.css"], ...);

would it work and make things simpler? (i.e. no or limited changes to the plugins?)

@wkeese
Copy link
Member

wkeese commented Nov 7, 2013

Note that dui/css! works that way already, but I agree, dui/themes/load! should be changed to take any path.

Note that themes/load! is more complicated that dui/css! because it takes a path like "Button.css" and converts it into "ios/Button.css, ios/Button_rtl.css".

Anyway, I was just saying that rearranging the directories isn't a trivial change, because we need to simultaneously update compile.js and load.js.

wkeese added a commit to wkeese/delite that referenced this issue Nov 26, 2013
Previously it required the CSS to be in the dui/themes directory.

Refs ibm-js#14 sort-of.
wkeese added a commit to wkeese/delite that referenced this issue Nov 27, 2013
Previously it required the CSS to be in the dui/themes directory.

Refs ibm-js#14 sort-of.
@wkeese
Copy link
Member

wkeese commented Nov 27, 2013

I'm working on having a separate directory per widget. The main issue is that each theme's variables.less file has data for all the widgets mangled together. While there are some general settings that should go into each theme's variables.less, such as "primary color" and "secondary color", things like @dui-slider-halo-size should presumably go into the Slider/ directory.

wkeese added a commit to wkeese/delite that referenced this issue Nov 29, 2013
- Require paths to load! to contain {{theme}} substitution variable.
- Remove RTL handling code from load!, punting the job to the widgets.
- Allow for other projects to "subclass" load.js w/a custom themeMap etc.

A widget that with both LTR and RTL CSS will have a define() dependency list including text like:

    "./themes/load!./themes/{{theme}}/StarRating",
    "dojo/has!dojo-bidi?./themes/load!./themes/{{theme}}/StarRating_rtl"

 Refs ibm-js#14.
wkeese added a commit to wkeese/delite that referenced this issue Nov 29, 2013
- Make plugin take arbitrary paths.  Paths should contain {{theme}} substitution variable.
- Remove RTL handling code from load!, punting the job to the widgets.
- Allow for other projects to "subclass" load.js w/a custom themeMap etc.

A widget that with both LTR and RTL CSS will have a define() dependency list including text like:

    "./themes/load!./themes/{{theme}}/StarRating",
    "dojo/has!dojo-bidi?./themes/load!./themes/{{theme}}/StarRating_rtl"

 Refs ibm-js#14.
wkeese added a commit to wkeese/delite that referenced this issue Nov 29, 2013
- Make plugin take arbitrary paths.  Paths should contain {{theme}} substitution variable.
- Remove RTL handling code from load!, punting the job to the widgets.
- Allow for other projects to "subclass" load.js w/a custom themeMap etc.

A widget that with both LTR and RTL CSS will have a define() dependency list including text like:

    "./themes/load!./themes/{{theme}}/StarRating",
    "dojo/has!dojo-bidi?./themes/load!./themes/{{theme}}/StarRating_rtl"

The order the CSS is inserted into the document is not guaranteed, so the selectors in
StarRating_rtl should have higher selectivity than the selectors in StarRating.

Refs ibm-js#14, fixes ibm-js#53.
wkeese added a commit that referenced this issue Nov 29, 2013
- Make plugin take arbitrary paths.  Paths should contain {{theme}} substitution variable.
- Remove RTL handling code from load!, punting the job to the widgets.
- Allow for other projects to "subclass" load.js w/a custom themeMap etc.

A widget with both LTR and RTL CSS will have a define() dependency list including entries like:

    "./themes/load!./themes/{{theme}}/StarRating",
    "dojo/has!dojo-bidi?./themes/load!./themes/{{theme}}/StarRating_rtl"

The order the CSS is inserted into the document is not guaranteed, so the selectors in
StarRating_rtl should override the selectors in StarRating.

Refs #14, fixes #53.
@ghost ghost assigned wkeese Dec 23, 2013
wkeese added a commit to wkeese/delite that referenced this issue Dec 23, 2013
Each widget subdirectory has:

- the WidgetName.less file previously in themes/common (ex: Button.less)
- multiple variables.less files (one per theme) defining settings for that widget (ex: ios/variables.less)
- all the compiled CSS for that widget (ex: ios/Button.js, holodark/Button_rtl.js).

The old themes/*/variables.less files have been trimmed to just contain properties useful
across multiple widgets, like @dark-color or @holo-color.

Fixes ibm-js#14.
wkeese added a commit to wkeese/delite that referenced this issue Dec 23, 2013
Each widget subdirectory has:

- the template for that widget (if it has a template)
- a themes/ directory, which contains:
	- a subdirectory for each theme, containing a WidgetName.less file
	  and its compiled version, WidgetName_css.js
	- a WidgetName_template.less file based on the old themes/common/WidgetName.less,
	  included by WidgetName.less files

The old themes/*/variables.less files have been trimmed to just contain properties useful
across multiple widgets, like @dark-color or @holo-color.

Fixes ibm-js#14.
wkeese added a commit to wkeese/delite that referenced this issue Dec 23, 2013
Each widget subdirectory has:

- the template for that widget (if it has a template)
- a themes/ directory, which contains:
	- a subdirectory for each theme, containing a WidgetName.less file
	  and its compiled version, WidgetName_css.js
	- a WidgetName_template.less file based on the old themes/common/WidgetName.less,
	  included by WidgetName.less files

The old themes/*/variables.less files have been trimmed to just contain properties useful
across multiple widgets, like @dark-color or @holo-color.

This commit includes a major refactoring of how less files are compiled.
Now, every compiled file (ex: Button_css.js) has as associated .less file.  This is opposed
to the old system where one template file in themes/common (ex: Button.less) was used to
compile multiple Button.js type output files.

Fixes ibm-js#14.
wkeese added a commit to wkeese/delite that referenced this issue Dec 24, 2013
Each directory has:

- the widget's template (if it has one)
- a themes/ subdirectory, which contains:
	- a subdirectory for each theme, containing a WidgetName.less file
	  and its compiled version, WidgetName_css.js
	- a WidgetName_template.less file based on the old themes/common/WidgetName.less,
	  included by WidgetName.less files

The old themes/*/variables.less files have been trimmed to just contain properties useful
across multiple widgets, like @dark-color or @holo-color.
In other words, this was mainly an exercise in splitting the less variables/macros per widget.

This commit includes a major refactoring of how less files are compiled.
Now, every compiled file (ex: Button_css.js) has as associated .less file.  This is opposed
to the old system where one template file in themes/common (ex: Button.less) was used to
generate multiple Button.js type output files.

Also, Slider's CSS had strange dependencies on the variables for Switch, etc.
Those dependencies were unrolled to make Slider independent.

Finally, transitions.css was importing CSS files that didn't exist, since the actual transition
CSS files were wrapped as JS files.  So I converted transitions.css to transitions.js.

Fixes ibm-js#14.
@wkeese wkeese closed this as completed in 4b72f0b Dec 24, 2013
wkeese added a commit that referenced this issue Dec 25, 2013
- Make plugin take arbitrary paths.  Paths should contain {{theme}} substitution variable.
- Remove RTL handling code from load!, punting the job to the widgets.
- Allow for other projects to "subclass" load.js w/a custom themeMap etc.

A widget with both LTR and RTL CSS will have a define() dependency list including entries like:

    "./themes/load!./themes/{{theme}}/StarRating",
    "dojo/has!dojo-bidi?./themes/load!./themes/{{theme}}/StarRating_rtl"

The order the CSS is inserted into the document is not guaranteed, so the selectors in
StarRating_rtl should override the selectors in StarRating.

Refs #14, fixes #53.
wkeese added a commit that referenced this issue Dec 25, 2013
Each directory has:

- the widget's template (if it has one)
- a themes/ subdirectory, which contains:
	- a subdirectory for each theme, containing a WidgetName.less file
	  and its compiled version, WidgetName_css.js
	- a WidgetName_template.less file based on the old themes/common/WidgetName.less,
	  included by WidgetName.less files

The old themes/*/variables.less files have been trimmed to just contain properties useful
across multiple widgets, like @dark-color or @holo-color.
In other words, this was mainly an exercise in splitting the less variables/macros per widget.

This commit includes a major refactoring of how less files are compiled.
Now, every compiled file (ex: Button_css.js) has as associated .less file.  This is opposed
to the old system where one template file in themes/common (ex: Button.less) was used to
generate multiple Button.js type output files.

Also, Slider's CSS had strange dependencies on the variables for Switch, etc.
Those dependencies were unrolled to make Slider independent.

Finally, transitions.css was importing CSS files that didn't exist, since the actual transition
CSS files were wrapped as JS files.  So I converted transitions.css to transitions.js.

Fixes #14.
wkeese added a commit that referenced this issue Dec 25, 2013
- Make plugin take arbitrary paths.  Paths should contain {{theme}} substitution variable.
- Remove RTL handling code from load!, punting the job to the widgets.
- Allow for other projects to "subclass" load.js w/a custom themeMap etc.

A widget with both LTR and RTL CSS will have a define() dependency list including entries like:

    "./themes/load!./themes/{{theme}}/StarRating",
    "dojo/has!dojo-bidi?./themes/load!./themes/{{theme}}/StarRating_rtl"

The order the CSS is inserted into the document is not guaranteed, so the selectors in
StarRating_rtl should override the selectors in StarRating.

Refs #14, fixes #53.
wkeese added a commit that referenced this issue Dec 25, 2013
Each directory has:

- the widget's template (if it has one)
- a themes/ subdirectory, which contains:
	- a subdirectory for each theme, containing a WidgetName.less file
	  and its compiled version, WidgetName_css.js
	- a WidgetName_template.less file based on the old themes/common/WidgetName.less,
	  included by WidgetName.less files

The old themes/*/variables.less files have been trimmed to just contain properties useful
across multiple widgets, like @dark-color or @holo-color.
In other words, this was mainly an exercise in splitting the less variables/macros per widget.

This commit includes a major refactoring of how less files are compiled.
Now, every compiled file (ex: Button_css.js) has as associated .less file.  This is opposed
to the old system where one template file in themes/common (ex: Button.less) was used to
generate multiple Button.js type output files.

Also, Slider's CSS had strange dependencies on the variables for Switch, etc.
Those dependencies were unrolled to make Slider independent.

Finally, transitions.css was importing CSS files that didn't exist, since the actual transition
CSS files were wrapped as JS files.  So I converted transitions.css to transitions.js.

Fixes #14.
wkeese added a commit that referenced this issue Dec 25, 2013
Each directory has:

- the widget's template (if it has one)
- a themes/ subdirectory, which contains:
	- a subdirectory for each theme, containing a WidgetName.less file
	  and its compiled version, WidgetName_css.js
	- a WidgetName_template.less file based on the old themes/common/WidgetName.less,
	  included by WidgetName.less files

The old themes/*/variables.less files have been trimmed to just contain properties useful
across multiple widgets, like @dark-color or @holo-color.
In other words, this was mainly an exercise in splitting the less variables/macros per widget.

This commit includes a major refactoring of how less files are compiled.
Now, every compiled file (ex: Button_css.js) has as associated .less file.  This is opposed
to the old system where one template file in themes/common (ex: Button.less) was used to
generate multiple Button.js type output files.

Also, Slider's CSS had strange dependencies on the variables for Switch, etc.
Those dependencies were unrolled to make Slider independent.

Finally, transitions.css was importing CSS files that didn't exist, since the actual transition
CSS files were wrapped as JS files.  So I converted transitions.css to transitions.js.

Fixes #14.
cjolif referenced this issue in ibm-js/sdk Mar 4, 2014
wkeese added a commit that referenced this issue Oct 23, 2014
- Make plugin take arbitrary paths.  Paths should contain {{theme}} substitution variable.
- Remove RTL handling code from load!, punting the job to the widgets.
- Allow for other projects to "subclass" load.js w/a custom themeMap etc.

A widget with both LTR and RTL CSS will have a define() dependency list including entries like:

    "./themes/load!./themes/{{theme}}/StarRating",
    "dojo/has!dojo-bidi?./themes/load!./themes/{{theme}}/StarRating_rtl"

The order the CSS is inserted into the document is not guaranteed, so the selectors in
StarRating_rtl should override the selectors in StarRating.

Refs #14, fixes #53.
wkeese added a commit that referenced this issue Oct 23, 2014
Each directory has:

- the widget's template (if it has one)
- a themes/ subdirectory, which contains:
	- a subdirectory for each theme, containing a WidgetName.less file
	  and its compiled version, WidgetName_css.js
	- a WidgetName_template.less file based on the old themes/common/WidgetName.less,
	  included by WidgetName.less files

The old themes/*/variables.less files have been trimmed to just contain properties useful
across multiple widgets, like @dark-color or @holo-color.
In other words, this was mainly an exercise in splitting the less variables/macros per widget.

This commit includes a major refactoring of how less files are compiled.
Now, every compiled file (ex: Button_css.js) has as associated .less file.  This is opposed
to the old system where one template file in themes/common (ex: Button.less) was used to
generate multiple Button.js type output files.

Also, Slider's CSS had strange dependencies on the variables for Switch, etc.
Those dependencies were unrolled to make Slider independent.

Finally, transitions.css was importing CSS files that didn't exist, since the actual transition
CSS files were wrapped as JS files.  So I converted transitions.css to transitions.js.

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

No branches or pull requests

4 participants