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

Detail tabs #66

Merged
merged 84 commits into from
Oct 2, 2017
Merged

Detail tabs #66

merged 84 commits into from
Oct 2, 2017

Conversation

sunglam
Copy link

@sunglam sunglam commented Sep 26, 2017

#minor#

CHANGELOG

  • Added detail tabs ( user can choose and add more tabs from a list of tabs).
  • A detail tab can have vertical sub tabs with sub tab's content area and related actions in vertical action bar.

@@ -1,8 +1,11 @@
/* eslint-env node */
const EmberAddon = require('ember-cli/lib/broccoli/ember-addon')
const EmberApp = require('ember-cli/lib/broccoli/ember-addon')
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert this change. This is an EmberAddon not an EmberApp.

Copy link
Author

@sunglam sunglam Sep 26, 2017

Choose a reason for hiding this comment

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

Sorry ..big mistake .. Thanks for this comment.. I is done


module.exports = function (defaults) {
var app = new EmberAddon(defaults, {
var app = new EmberApp(defaults, {
Copy link
Contributor

Choose a reason for hiding this comment

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

EmberAddon instead of EmberApp.

Copy link
Author

Choose a reason for hiding this comment

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

Done

package.json Outdated
"ember-load-initializers": "^0.6.0",
"ember-load-initializers": "0.6.3",
"ember-math-helpers": "2.1.0",
"ember-elsewhere": "1.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

This got moved but is now not in alphabetical order. Please alphabetize.

Copy link
Author

Choose a reason for hiding this comment

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

Done

package.json Outdated
"ember-export-application-global": "^1.0.5",
"ember-disable-proxy-controllers": "^1.0.1",
"ember-element-resize-detector": "0.1.5",
"ember-export-application-global": "^1.1.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please leave this at: "ember-export-application-global": "^1.0.5",

Copy link
Author

Choose a reason for hiding this comment

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

Done

package.json Outdated
@@ -48,10 +48,14 @@
"ember-computed-decorators": "0.3.0",
"ember-concurrency": "0.7.19",
"ember-disable-prototype-extensions": "^1.1.0",
"ember-elsewhere": "1.0.1",
"ember-export-application-global": "^1.0.5",
"ember-disable-proxy-controllers": "^1.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be necessary since this has been gone since Ember 2.0: https://github.com/cibernox/ember-disable-proxy-controllers#ember-disable-proxy-controllers

Copy link
Author

Choose a reason for hiding this comment

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

Done

})
})

it('should have slected tab by default', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Spelling: slected should be selected.

Copy link
Author

Choose a reason for hiding this comment

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

done.

_subtabs (subtabs) {
if (isEmpty(subtabs) || typeOf(subtabs[0]) === 'object' || typeOf(subtabs[0]) === 'instance') {
// const isMissingProperties = subtabs.some(({id, label, icon, pack}) => {
// return !id || !label
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the commented code if it is not needed.

Copy link
Author

Choose a reason for hiding this comment

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

Done


// == Computed Properties ===================================================

// Wrap selectedTab for the same reasons outlined in the comments for _tabs
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the comment for _tabs that this comment references still in the code base? What happens if that comment were to be deleted or is already deleted? Instead, it would be better to add the comment block here as well.

Copy link
Author

Choose a reason for hiding this comment

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

Done

// This is annoying, but the math appears to be slightly off, maybe because of the border
// between the tabs - so we're dropping one off the tabLeft for the border.
// Why the viewport needs another pixel for this to calculate correctly, I don't know...
// TODO Find out why...
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put your username and date against this TODO and open an issue as well.

Copy link
Author

Choose a reason for hiding this comment

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

Done

// == Keyword Properties ====================================================

layout,
tagName: '',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be changed to a span maybe?

Copy link
Author

Choose a reason for hiding this comment

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

Done

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling ca019b5 on sunglam:detail-tabs into ** on ciena-frost:master**.

@rox163
Copy link
Contributor

rox163 commented Sep 29, 2017

👍

Approved with PullApprove

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.

None yet

6 participants