feat(tabs): Implementation #138

Vignesh-Manogar-E3433 · 2020-02-26T10:06:59Z

Description

Adding 'Tabs' component to Nucleus.

Tabs are used to organise content under each section. Tabs are most helpful when there is a lot of content to show in a page. Tabs can help in showing content which are under the same level of hierarchy, under each section inside the same page.

Related Bug

Basic functionality working Refractoring some code

netlify · 2020-03-09T06:08:08Z

Deploy preview for freshworks-nucleus processing.

Building with commit dba6afd

https://app.netlify.com/sites/freshworks-nucleus/deploys/5e73781d068d3a0007d29b35

shibulijack-fd · 2020-03-10T12:16:58Z

packages/tabs/addon/components/nucleus-tabs/tab-panel.js

+  * @type boolean
+  * @public
+  */
+  @computed('props.[]', function() {


Instead of computing the values inside props, we could directly pass the prop elements from the parent component and consume accordingly

Done. Now directly passing the values.

shibulijack-fd · 2020-03-10T12:20:03Z

packages/tabs/addon/templates/components/nucleus-tabs.hbs

+
+{{yield (hash
+  panel=(component "nucleus-tabs/tab-panel")
+  props=(hash


While I feel it's a little strong to be called anti-pattern (as some Ember devs would), I would say it's not the Ember convention to send props like this. Also, this implies that you have to watch for any changes in the props hash even to listen for one of the values which could have performance implications.

Yes, makes sense. Changed as suggested.

shibulijack-fd · 2020-03-10T12:20:45Z

packages/tabs/addon/components/nucleus-tabs/tab-list-item.js

+  @extends Ember.Component
+  @private
+*/
+@tagName('button')


Please use {{nucleus-button}}

Tab and Button have completely different aria attributes, roles. Styling and behaviour also differs. Hence just changed the base tag as per our discussion to avoid any confusions.

shibulijack-fd · 2020-03-10T12:21:16Z

packages/tabs/addon/components/nucleus-tabs/tab-list-item.js

+  */
+  init() {
+    super.init(...arguments);
+    once(this, get(this, 'registerTabListItem'), {


Is a run loop necessary here?

Removed using run loops. Was able to achieve the same result by using lifecycle event. Updated.

shibulijack-fd · 2020-03-10T12:23:30Z

packages/tabs/addon/styles/components/_nucleus-tabs.scss

+      border-radius: 4px;
+    }
+  }
+}


Can you please refactor this file using BEM?
https://css-tricks.com/bem-101/

Was already following BEM naming conventions. Have changed a couple of lines to be more compliant.

shibulijack-fd · 2020-03-10T12:26:59Z

packages/tabs/tests/integration/components/nucleus-tabs-test.js

+      {{/nucleus-tabs}}
+    `);
+    await backstop(assert, {scenario:{misMatchThreshold: 0.1}}); 
+  });


Visual regression tests are expensive, i.e they take a long time to run and taking snapshots are quite costly too. So try to render multiple variations in one test as much as possible.

Merged visual regression test cases into one as suggested.

Merging cases into one also led to reducing backstop 'misMatchThreshold'. Something to be wary of next time. Else, cases were passing when they should have failed.

Merging cases into one also led to reducing backstop 'misMatchThreshold'. Something to be wary of next time.

I have already brought this up in an issue - #143
We are currently letting the developer be the better judge of the misMatchThreshold depending on the way they have written the test. It is not necessary that all visual regression tests should have the same misMatchThreshold. We will have to revisit the already existing components too and change threshold values if required

shibulijack-fd · 2020-03-19T06:46:14Z

packages/tabs/addon/components/nucleus-tabs/tab-list-item.js

+  * @private
+  *
+  */
+  _focusPreviousTab(element, elementInFocus) {


This function is almost the same as _focusNextTab. So can you refactor this into a single function with an additional param to address both the use cases?

Their primary functionality is the same, but they simply operate on different nodes. Combining them makes more mess than solve something.

_focusTab(element, elementInFocus, direction) { let siblingElement; if(direction) { siblingElement = (element.nextElementSibling)? element.nextElementSibling : element.parentElement.firstElementChild; } else { siblingElement = (element.previousElementSibling)? element.previousElementSibling : element.parentElement.lastElementChild; } if(elementInFocus && (elementInFocus.id === siblingElement.id)) { return; } else if(siblingElement.getAttribute('tabindex') === null) { get(this, '_focusTab').call(this, siblingElement, element, direction); } else { siblingElement.focus(); } }

D.R.Y: Don't Repeat Yourself.

Having two functions with almost the same lines of code is a prime candidate to apply this principle.

shibulijack-fd · 2020-03-19T06:47:06Z

packages/tabs/addon/components/nucleus-tabs/tab-panel.js

+@classNameBindings('isActive:is-active')
+@attributeBindings('tabindex')
+@attributeBindings('role')
+@attributeBindings('aria-labelledby')


@attributeBindings accepts a list of values. So this can be refactored to a single line.

shibulijack-fd · 2020-03-19T06:47:42Z

packages/tabs/addon/components/nucleus-tabs/tab-panel.js

+  *
+  */
+  init() {
+    super.init(...arguments);


I think this lifecycle hook can be removed.

shibulijack-fd · 2020-03-19T06:48:27Z

packages/tabs/addon/components/nucleus-tabs/tab-list-item.js

+@attributeBindings('aria-controls')
+@attributeBindings('aria-selected')
+@attributeBindings('isDisabled:disabled')
+@attributeBindings('title')


Please refactor to single binding calls.

shibulijack-fd · 2020-03-19T06:50:01Z

packages/tabs/addon/components/nucleus-tabs.js

+  */
+  @action
+  async activateTab(changedTo, event) {
+    const _this = this;


Is this necessary? I don't see any scope changes within this function?

shibulijack-fd · 2020-03-19T06:50:43Z

packages/tabs/addon/components/nucleus-tabs.js

+    const _this = this;
+    const beforeChange = get(_this, 'beforeChange');
+    const onChange =  get(_this, 'onChange');
+    const currentTab = get(_this, 'selected');


You could use getProperties to batch the getter jobs.

https://api.emberjs.com/ember/release/functions/@ember%2Fobject/getProperties

shibulijack-fd · 2020-03-19T06:52:55Z

packages/tabs/addon/components/nucleus-tabs/tab-list-item.js

+  *
+  */
+  mouseDown() {
+    if(get(this, 'isDisabled') === false) set(this, 'isPressed', true);


Not sure if there's a linter rule for this, but can you wrap it in { }

mouseDown() { if(get(this, 'isDisabled') === false) { set(this, 'isPressed', true); } }

P.S: It's just a convention that we follow in our codebase.

Sure, will follow this hereon.

…ine if statements wrapped

shibulijack-fd · 2020-03-19T08:28:11Z

packages/tabs/addon/components/nucleus-tabs/tab-list-item.js

+  * @private
+  *
+  */
+  _focusPreviousTab(element, elementInFocus) {


_focusTab(element, elementInFocus, direction) { let siblingElement; if(direction) { siblingElement = (element.nextElementSibling)? element.nextElementSibling : element.parentElement.firstElementChild; } else { siblingElement = (element.previousElementSibling)? element.previousElementSibling : element.parentElement.lastElementChild; } if(elementInFocus && (elementInFocus.id === siblingElement.id)) { return; } else if(siblingElement.getAttribute('tabindex') === null) { get(this, '_focusTab').call(this, siblingElement, element, direction); } else { siblingElement.focus(); } }

shibulijack-fd · 2020-03-19T08:29:11Z

packages/tabs/addon/components/nucleus-tabs/tab-list-item.js

+  * @private
+  *
+  */
+  _focusPreviousTab(element, elementInFocus) {


D.R.Y: Don't Repeat Yourself.

Having two functions with almost the same lines of code is a prime candidate to apply this principle.

* master: fix(modal): fixes positioning of elements (#154) fix(toast-message): Design review updates (#155) feat(tabs): Implementation (#138) chore(nucleus): Nucleus now lives in the Dark! (#152) fix(Inline-banner): refactored yield (#151) # Conflicts: # packages/@nucleus/package.json

feat(tabs): Initial commit

024d385

Vignesh-Manogar-E3433 requested a review from shibulijack-fd February 26, 2020 10:06

weekly-digest bot mentioned this pull request Mar 1, 2020

Weekly Digest (23 February, 2020 - 1 March, 2020) #142

Closed

Vignesh-Manogar-E3433 added 10 commits March 3, 2020 14:14

feat(tabs): Implemented {{nucleus-tabs}}

feac2a3

Basic functionality working Refractoring some code

refactor(tabs): calling component attributes through get call

5a84fa7

fix(tabs): Background variant border radius issue

6df0f48

refractor(tabs): current selected name change

f590917

fix(tabs): no focus on disabled button

17dcb82

fix(tabs): no focus on disabled button & safari text rendering issue

907fda0

feat(tabs): 'beforeChange' handler implementation

d6015a8

feat(tabs): Adding customClasses prop to tabs

0d3c03d

refractor(tabs): class names changed to follow existing conventions

8167111

refractor(tabs): Added missing empty lines at the end of files

a177a2d

Vignesh-Manogar-E3433 requested a review from rajasegar-c March 6, 2020 12:16

Vignesh-Manogar-E3433 marked this pull request as ready for review March 6, 2020 12:16

shibulijack-fd changed the title ~~feat(tabs): Initial commit~~ feat(tabs): Implementation Mar 9, 2020

shibulijack-fd added the enhancement New feature or request label Mar 9, 2020

Merge branch 'master' into feature/nucleus-tabs

68555b1

Vignesh-Manogar-E3433 and others added 3 commits March 9, 2020 15:19

fix(tabs): documentation issues

d4b5ade

Merge branch 'master' into feature/nucleus-tabs

37470bf

fix(tabs): safari font color on active element

a4f3033

shibulijack-fd suggested changes Mar 10, 2020

View reviewed changes

Vignesh-Manogar-E3433 and others added 4 commits March 11, 2020 17:42

refractor(tabs): Removed sending attributes in single props object

ad5af24

feat(tabs): select prop optional

0606c98

feat(tabs): disabled as boolean or string

21b017e

Merge branch 'master' into feature/nucleus-tabs

4aafed1

Vignesh-Manogar-E3433 self-assigned this Mar 18, 2020

Vignesh-Manogar-E3433 added 2 commits March 18, 2020 16:09

fix(tabs): focus styling not appearing on click

d358915

fix(tabs): removed vendor prefixes

b3867bb

fix(tabs): pressed and active hover state styling

678ebfa

shibulijack-fd suggested changes Mar 19, 2020

View reviewed changes

refactor(tabs): Combined attribute bindings, class bindings. Single l…

c4d7676

…ine if statements wrapped

shibulijack-fd suggested changes Mar 19, 2020

View reviewed changes

Vignesh-Manogar-E3433 and others added 3 commits March 19, 2020 14:49

refactor(tabs): changeFocusTab function

f9086e2

Merge branch 'master' into feature/nucleus-tabs

fcce8cc

Merge branch 'master' into feature/nucleus-tabs

dba6afd

shibulijack-fd approved these changes Mar 19, 2020

View reviewed changes

shibulijack-fd merged commit 598e3e6 into master Mar 19, 2020

shibulijack-fd deleted the feature/nucleus-tabs branch March 19, 2020 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tabs): Implementation #138

feat(tabs): Implementation #138

Vignesh-Manogar-E3433 commented Feb 26, 2020

netlify bot commented Mar 9, 2020 •

edited

Loading

shibulijack-fd Mar 10, 2020

Vignesh-Manogar-E3433 Mar 11, 2020

shibulijack-fd Mar 10, 2020

Vignesh-Manogar-E3433 Mar 11, 2020

shibulijack-fd Mar 10, 2020

Vignesh-Manogar-E3433 Mar 11, 2020

shibulijack-fd Mar 10, 2020

Vignesh-Manogar-E3433 Mar 11, 2020

shibulijack-fd Mar 10, 2020

Vignesh-Manogar-E3433 Mar 11, 2020

shibulijack-fd Mar 10, 2020

Vignesh-Manogar-E3433 Mar 11, 2020 •

edited

Loading

sharath-sriram Mar 19, 2020

shibulijack-fd Mar 19, 2020

Vignesh-Manogar-E3433 Mar 19, 2020

shibulijack-fd Mar 19, 2020

shibulijack-fd Mar 19, 2020

Vignesh-Manogar-E3433 Mar 19, 2020

shibulijack-fd Mar 19, 2020

Vignesh-Manogar-E3433 Mar 19, 2020

shibulijack-fd Mar 19, 2020

Vignesh-Manogar-E3433 Mar 19, 2020

shibulijack-fd Mar 19, 2020

Vignesh-Manogar-E3433 Mar 19, 2020

shibulijack-fd Mar 19, 2020

Vignesh-Manogar-E3433 Mar 19, 2020

shibulijack-fd Mar 19, 2020

Vignesh-Manogar-E3433 Mar 19, 2020

shibulijack-fd Mar 19, 2020

Vignesh-Manogar-E3433 Mar 19, 2020

shibulijack-fd Mar 19, 2020

shibulijack-fd Mar 19, 2020

feat(tabs): Implementation #138

feat(tabs): Implementation #138

Conversation

Vignesh-Manogar-E3433 commented Feb 26, 2020

Description

Related Bug

netlify bot commented Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vignesh-Manogar-E3433 Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Mar 9, 2020 •

edited

Loading

Vignesh-Manogar-E3433 Mar 11, 2020 •

edited

Loading