Allow custom semantic-ui builds

[khornberg]

Adds two configuration options. One for the path to the dist folder of
Semantic. The other option to specify the theme used for images and
fonts.

Note that this does not add support for importing files outside of the
build tree (e.g. bower_components). One can use bower link for local
projects.

[aaronbhansen]

Thanks, I should get a chance to look into these in the next couple of days.

[v3ss0n]

This is awesome , really needed. This will reduce a lot of pain in choosing themse. Please merge

[aaronbhansen]

The hesitation was mostly around not having unit tests to verify changes for this and for the future. I had an idea tonight, if we could pull the app import methods into a utilities class, then we could import that class to do the calls and test the results.

@khornberg would you be open to added that to this pull request? If so I can detail out more what I'm thinking

[khornberg]

@aaronbhansen Sure, I would like to add tests if you have something in mind

[khornberg]

@aaronbhansen I'm not sure what you were thinking; I am not sure how one can test if the proper file is included in a build of ember. I have thought about how to test the the function that gives us the paths to import.

//index.js

from utils import 'importPaths';

included: function(app) {
    paths = importPaths(app.options['SemanticUI']); // returns an array of strings or objects ['', '', {}, {}]
    paths.forEach((path) => { app.import(path); });
}

//utils/importPaths.js

// here do the if checks for each option and append to array

[v3ss0n]

seems all good ?

[aaronbhansen]

@khornberg sorry for the delay in responding, this weeks been crazy busy. A quick overview of my idea was to move the url generation into a utils file and then test that. I should get time this weekend to explain a little more and provide an update.

[khornberg]

Ok, no worries. I'll push what I've worked on today.

A few hours ago I realized that the index.js is outside of babel and ember.

[khornberg]

@v3ss0n It is ready to test now

[khornberg]

CI is failing because contains is deprecated in 2.8 in favor of includes.

Semantic-UI-Ember/addon/mixins/base.js

Line 38 in 99086c2

if (settableProperties.contains(key) && gettableProperties.contains(key)) {

[aaronbhansen]

I've never used import like that before. I thought it was always a string or an object. I couldn't see if in the documentation either. Can you point me to where this is supported instead of an object?

[aaronbhansen]

Nevermind, I see what you're doing below. I missed that before.

[aaronbhansen]

Sorry for the delay in responding, I think its fine to leave most the code in index.js, I think that separates out concerns better. Mostly what I wanted to test was to pull out the default code and some simple formatting code into a separate util, test that, and then import that util into the index.js to produce the final result. As an example index.js could look like

var generate  = require('./utils/generation');

....
// A check that ensures the options passed in aren't empty, relative to ember isPresent
var theme = generation.default('path', 'theme', [options, defaults]);

if (generate.allow('imports', 'images', [options, defaults])) {
  var imageOptions = {
    destDir: generate.format('assets/themes', theme, 'assets/images')
  };
 app.import(generate.format(path, 'themes', theme, 'assets/images/flags.png'), imageOptions);
}

The generation util has three new methods we can test. Those are

default (method):

Parameters
1. The nested path
2. The property name
3. Any properties to consider defaulting in order

The first argument default finds that exists on the objects passed in is returned. The object could be null or empty string. We just need to check if the key exists on the object.

Now that we have multiple settings, it would be nice to nest them in the object. Have the app settings look like

SemanticUI: {
  imports: {
    fonts: false,
    images: false
  },
  paths: {
    theme: 'something'
  }
}

but we also need to support a flat version for older projects.

SemanticUI: {
  fonts: false,
  images: false,
  paths: {
    theme: 'something'
  }
}

In essence what the default method does is try to first resolve any variables that exist on the full path first, then the fall back, then the default full path, then the default fallback.

The check would be something like

• check imports/fonts on options
• check fonts on options (if found here, log out deprecation warning)
• check imports/fonts on defaults
• check fonts on defaults (if found here, log out deprecation warning)

The next method would be

import (method):

Parameters
1. The nested path
2. The property name
3. Any properties to consider defaulting in order

Same functionality and idea as default, but its specifically looking for a true / false value instead of first argument found on the object.

Last method would be

format (method):

Parameters
* An array of values

The reason we allow null or empty strings on default is for here. Lets say that they don't want to put the theme assets under theme. They could do this.

SemanticUI: {
  paths: {
    theme: null
  }
}

The generation would look like
var theme = generation.default(...) // theme is null

var destination = generate.format('assets/themes', theme, 'assets/images');
// result = 'assets/themes/assets/images'

the format method checks each value in the array and if the value is null, undefined, or whitespace, it removes it from the array.

Then we just do a simple [remaining values].join('/') to return the value.

Wrapping up

In the end, if we can create those three methods, and have tests against them, that will allow us to have much more flexibility in the future and start to allow multiple configuration options for anyones needs.

Let me know what you think

[khornberg]

@aaronbhansen Ok

Do you have a strong preference of qunit over another testing framework? I've not worked with qunit on node outside of ember. Thus far, I've had to install it separately from ember-qunit to get the binary.

The tests seem fairly straight forward so I think either one would work fine.

[aaronbhansen]

I don't necessarily love qunit, but its the default ember path, and its the path of less resistance. I'd rather just use qunit than have two testing frameworks installed.

Here is a helper with the default qunit test for non ember utils.

import { aOrAn } from 'shared/helpers/a-or-an';
import { module, test } from 'qunit';

module('Unit | Helper | a or an');

// Replace this with your real tests.
test('it works', function(assert) {
  let result = aOrAn(['test']);
  assert.ok(result);
});

[khornberg]

@aaronbhansen Yep don't disagree. As far as I understand, index.js is not run through babel or run within the context of ember. Are you thinking that that file live in addon/util/ and then when ember tests are run it would also be tested?

[aaronbhansen]

Thats correct, looks like the path can be anywhere from the other examples I've seen.

https://github.com/emberjs/data/blob/master/index.js#L110

[khornberg]

@aaronbhansen How would the imports work with ember expecting exports default function generation()... and node expecting modules.export = function... from index.js as require('addon/utils/generation')?

Maybe I'm missing something but those are not working for me.

[khornberg]

@aaronbhansen I don't think the import or allow method is necessary. Are they the same thing? allow was in your index.js example but seems to be doing what you described under import.

I've updated the index.js to a working version.

In manual tests of ember build everything seems to check out.

[aaronbhansen]

Yeah after looking at it, they aren't needed. Initially I was thinking of it differently, but since all the imports are bools, it doesn't matter. Lets switch over to qunit and I think thats enough to merge in. Then we can update your other pull request on documentation with the new recommend structure.

[khornberg]

@aaronbhansen I can certainly try to add qunit to the addon if desired. It would have to be installed since ember-cli-qunit installs qunitjs which is not the same as qunit. From my understanding qunit installs a binary that can be called from the command line. I thought I had it runner earlier but I cannot get to run now in the 15 minutes I tried.

Since we have to add something anyways to the add for these tests what do you think we should do?

Do you happen to know how to add qunit tests in node?

Is there anything I need to add so that tests are run on CI?

[aaronbhansen]

I don't think it needs to be installed separately. We test utils just like we are doing in this repo with just ember-qunit installed. I believe that package adds qunit as a dependency. Here is a screenshot of our actual utils tested in a shared repo. https://cl.ly/0m0a0M1s1T2d

[khornberg]

What does computed-sum-by look like? I don't understand how it can be imported. My knowledge of node modules is lacking.

As I understand it, index.js does not have the ability to have import generator from "utils/generator" since it is not transpiled and runs on node 4. index.js uses module.exports, which I understand to mean that it is a commonjs module, not an es6 module.

[aaronbhansen]

It can't import like that, but you can do require('addon/utils/generator') in index.js. And as long as it doesn't have any es6 code, it should import just fine. Then on the flip side you could import or require it yet again in the tests to test the code. The computed sum by does have import statements, thats only because its only used within es6 code, but how generator is today it should be fine.

[aaronbhansen]

With that said, if you're stuck, I can merge in and then finish making the changes later this weekend or monday.

[khornberg]

After trying a few things, I'm still not getting things to import.
I am curious how this all works out though.

I've rebased to master and a single commit.

From what I understand everything should be fairly easy to make testable with qunit.

I've stuck everything in /utils

[khornberg]

@aaronbhansen I've checked the Allow edits from maintainers. Not sure how well that will work but hey it's new :)

[khornberg]

@aaronbhansen This is passing again and rebased to master.

I tried again to require(something) in an ember test unsuccessfully. If you have an example of how to do what you are thinking please let me know. I don't mind hooking it up that way. I'm out of ideas and I'm not sure how it would work to mix commonjs require and es6 imports.

[aaronbhansen]

Thank @khornberg I finally got time to start working on this again last night. Last week got a bit too busy. I'm going to update ember and dependencies, then look at getting this merged in and released this week.

[khornberg]

Cool, totally understand

[v3ss0n]

Ok can't wait to test.

[aaronbhansen]

Thanks, going to pull this in an finish the changes on master 👍

[v3ss0n]

Awesome , gonna test tomorrow.

[aaronbhansen]

@khornberg take a look at this commit 9cc8eed.
I started looking at it last night and realized that it would probably be easiest if the full path was totally customizable. I think that makes it easier to understand and configure now.

Also, I couldn't get it to work on es6 imports either. It was creating a loop when I did. I figured tape was the easiest and set it up like i've seen on other projects with node-tests

[khornberg]

@aaronbhansen Thanks, those changes add clarity; everything seems to work fine with the customization.

[aaronbhansen]

Glad to hear it worked. Thanks for all the work you did on it.