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

Bugfix: use addon config options #238 #242

Merged
merged 1 commit into from
Jan 3, 2019

Conversation

jenweber
Copy link
Contributor

@jenweber jenweber commented Jan 2, 2019

What this PR does

It fixes a bug where the consuming app's options set in ember-cli-build were not being read by this addon.

The problem:

The ember-cli-build.js configuration options were not being used at all by the addon due to a logic mistake in the JavaScript. For example, the css boolean below made no difference, and the addon kept using the default styles. Likewise, naming specific source paths had no effect.

// this didn't work
var app = new EmberApp(defaults, {
  // Add options here
  SemanticUI: {
    import: {
      css: false,
    }
  }
});

Here's the problem in index.js. options evaluates to true or {}, when it should evaluate to to an object of the options, like {import: { css: false } }. getDefault is a function that tries to traverse the options object, but since options is a boolean, it falls back to the addon defaults.

    const options = (app && app.options['SemanticUI']) || (app && app.options['semantic-ui-ember']) || {};
//...
    const importCss = getDefault('import', 'css', [options, defaults])
    if (importCss) {
      const sourceCss = getDefault('source', 'css', [options, defaults])
      app.import({
        development: path.join(sourceCss, 'semantic.css'),
        production: path.join(sourceCss, 'semantic.min.css')
      });
    }

The fix

Do the logical check, and then assign the value of options:

    let options;
    if (app && app.options['SemanticUI']) {
      options = app.options['SemanticUI']
    } else if (app && app.options['semantic-ui-ember']) {
      options = app.options['semantic-ui-ember']
    } else {
      options = {}
    }

I added a test for this. It can be run with node tape-tests/utils/get-default-test.js.

Bug replication & solution example app

Here's how to replicate the bug. Inspect ember-cli-build.js of this example app, run it locally, and you'll see that the button styles are Semantic's, when the css should be disabled according to the build options.

git clone git@github.com:jenweber/semantic-ui-ember-demo.git
cd semantic-ui-ember-demo
npm install
ember serve

screen shot 2019-01-02 at 4 06 19 pm

Here's the solution in action. Try out the app while using my fork with the bugfix and you will see the styles are disabled as they should be:

git checkout bugfix-example
npm install
ember serve

screen shot 2019-01-02 at 4 04 32 pm

P.S. thanks for your work on this addon!

Closes Semantic-Org#238
There was a logic problem where "options" passed to get-defaults
was a boolean, when it should have been the contents of the
options object passed through the consuming app's ember-cli-build.
@codecov-io
Copy link

codecov-io commented Jan 2, 2019

Codecov Report

Merging #242 into master will not change coverage.
The diff coverage is 66.66%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #242   +/-   ##
=======================================
  Coverage   81.52%   81.52%           
=======================================
  Files          15       15           
  Lines         525      525           
  Branches       24       24           
=======================================
  Hits          428      428           
  Misses         97       97
Impacted Files Coverage Δ
index.js 75.88% <66.66%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d5f93c...ace9b5a. Read the comment docs.

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

Successfully merging this pull request may close these issues.

3 participants