COR-680: Tenant Switcher #522

MKwenhua · 2017-09-01T16:42:06Z

Purpose:

Have a tenant switcher so that users can easily change the Cortex Tenant like CircleCI

JIRA:

https://cb-content-enablement.atlassian.net/browse/COR-680

Steps to Take On Prod

run migrations on the database

Changes:

Changes to setup
- run bundle exec rake db:migrate
Architectural changes
*
Migrations
- yes I added a new attribute on User active_tenant
Library changes
- yes I made changes to cortex-plugins
Side effects
*

Screenshots

Before
After

QA Links:

http://web.cortex-3.development.c66.me/

How to Verify These Changes

Specific pages to visit
*
Steps to take

go to http://web.cortex-3.development.c66.me/
try and switch tenants
make sure the UI responds as it's supposed to.
refresh the page and make sure the Tenant change has persisted.

Responsive considerations
*

Relevant PRs/Dependencies:

cortex-cms/cortex-plugins-core#60

Additional Information

A few things to note: classes like .sidebar--tentant-display are temporary and should be removed once both the side bar and the content area are rendered within React.

…for displaying tenant list

…er to hold sidenav classes

…Rails.authenticityToken() api to obtain CSRF token

…e object

toastercup · 2017-09-05T19:26:53Z

client/app/bundles/dashboard/components/side_bar/tenant_list.jsx

+} from 'dashboard/elements/loaders'
+
+const TenantItem = ({name, subdomain}, tenantClicked) => (
+  <li key={subdomain} className="mdl-list__item" onClick={tenantClicked}>


I think you'd need to add the button role (among other things, such as tabindex) to make this accessible: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_button_role

More important for Employer than Cortex, but accessability issues are something we want to avoid regardless

Ok I will add

Good catch here! Could the contents of the li be a button or a tag instead? Then you wouldn't need the button role. Before adding ARIA roles consider if there is a way to make the HTML more semantic

@arelia I just used the mdl https://getmdl.io/components/index.html#lists-section examples for list items and if I were to add a button inside of the list item I would have to add extra styling to override the default browser stylings and well as account for potential overriding of any default behaviors. I just tried wrapping the list items and it would require a lot of extra styling:

button

a-tag

…tainer to remove sidebar--tenant-display class from #layout_wrapper on successful tenant switch

… method to index and create a lookup object for tenants by organization

…enant to tenant if null

…on_displayed back to selected_tenant organization

toastercup · 2017-09-06T15:10:51Z

@MKwenhua The main issue I'm seeing so far is that there's no way to select a top-level tenant/org - if you click it, it expands to display sub-tenants, but does not allow you to select the top-level tenant itself. Most of our users will be interacting with top-level tenants, so this we'll need to sort out how this works prior to merge. Let's whiteboard this and discuss.

MKwenhua · 2017-09-06T15:17:52Z

The main issue I'm seeing so far is that there's no way to select a top-level tenant/org - if you click it, it expands to display sub-tenants, but does not allow you to select the top-level tenant itself. Most of our users will be interacting with top-level tenants, so this we'll need to sort out how this works prior to merge. Let's whiteboard this and discuss.

@toastercup Oh yeah! How did I miss that, when you get the time let's discuss the implementation details

arelia

I don't think I need to re-review for any of my comments, since all of this is temporary. The only thing I'm really concerned about is making sure there are lis nested in the ul tags and not spans

arelia · 2017-09-06T15:49:19Z

app/assets/javascripts/sidebar-toggle.js

-});
+// $(document).ready(function(){
+//   $('#sidebar__toggle-button').on('click', function() {
+//     $('body').toggleClass('sidebar--collapsed');


Either way is fine with me, but since you've commented this out, if you're planning to bring this back later should the class name go back to being .layout__container so that it works if you un-comment this?

Oh actually I was planning on removing that code completely, but yeah I have changed it to having a parent conatianer over .layout__container called #layout_wrapper which is used for the three Dashboard layout states 'no class', .sidebar--collapsed, and .sidebar--tentant-display

arelia · 2017-09-06T15:52:28Z

app/assets/stylesheets/components/loaders.scss

+  height: 11em;
+  border-radius: 50%;
+  background: $color-teal;
+  background: -moz-linear-gradient(left, $color-teal 10%, rgba(0,255,255, 0) 42%);


this doesn't reallly matter since all of this will eventually be redone, but Cortex only supports newer browsers and newer browsers don't need the prefixed version of linear-gradient, so you only need line 22

same for animation transform and @keyframes

I didn't write this I just copied and pasted the css code I found here: https://projects.lukehaas.me/css-loaders/ pretty much all of the styles I have added are just temporary placeholders.

But with that said I will go back and remove the vendor prefixes.

arelia · 2017-09-06T16:09:05Z

client/app/bundles/dashboard/components/side_bar/environment_flag.jsx

+    if (environment === 'production') return null
+    const flagClass = `mdl-navigation__link nav__item environment environment--${environment}`
+    return (
+      <div className={flagClass}>


instead of rendering both divs, could you wrap them in an if/else?

Technically yes, but already in the CSS . environment__full is hidden and . environment__abbreviated is displayed when it has a parent with .sidebar--collapsed.

👍 ok that's fine, it was just a thought since you're adding React

Oh yeah, but I realized the logic was already accounted for so I just rendered out the HTML. Glad that the logic was already there so that I wouldn't have to account for it when creating the components

arelia · 2017-09-06T16:12:52Z

client/app/bundles/dashboard/components/side_bar/tenant_list.jsx

+} from 'dashboard/elements/loaders'
+
+const TenantItem = ({name, subdomain}, tenantClicked) => (
+  <li key={subdomain} className="mdl-list__item" onClick={tenantClicked}>


Good catch here! Could the contents of the li be a button or a tag instead? Then you wouldn't need the button role. Before adding ARIA roles consider if there is a way to make the HTML more semantic

arelia · 2017-09-06T16:18:31Z

client/app/bundles/dashboard/components/side_bar/tenant_list.jsx

+      let {org_tenant, sub_tenants } = OrganizationLookup[org_id]
+      return (
+        <span key={org_tenant.subdomain + '_org'}>
+         <li role="button" className={ "mdl-list__item organization--label" + (organization_displayed === org_tenant.id ? ' active' : '' ) } tabIndex={index} onClick={organizationClicked(org_id)}><strong>{org_tenant.name}</strong></li>


like the comment I added above, if you can make the content of the li be a button that's better than adding role="button". Take a look at the Featured Items on the HiringDot homepage, which are styled to not look like buttons at all

arelia · 2017-09-06T16:19:22Z

client/app/bundles/dashboard/components/side_bar/tenant_list.jsx

+    return OrganizationLookup.organizations.map((org_id, index) => {
+      let {org_tenant, sub_tenants } = OrganizationLookup[org_id]
+      return (
+        <span key={org_tenant.subdomain + '_org'}>


Since these are going to get rendered into a ul, which can only take lis as children, can you move the key attribute to the li tag?

Add the subtenent ul before the closing li tag (because it's a list nested inside a list, right?), or move it into it's own component if you have to (if it's not meant to be nested), and that way you'll still have everything wrapped inside of one tag

@arelia Ok good catch! I changed it to UL instead.

@MKwenhua I don't think the span needs to be a ul, just move the </li> so that is is after the </ul> and remove the spans, like this

return ( <li key={org_tenant.subdomain + '_org'} role="button" className={ "mdl-list__item organization--label" + (organization_displayed === org_tenant.id ? ' active' : '' ) } tabIndex={index} onClick={organizationClicked(org_id)}> <strong>{org_tenant.name}</strong> <ul className={ organization_displayed === org_tenant.id ? "demo-list-icon mdl-list" : "hidden"}> { sub_tenants.map((tenant) => TenantItem(tenant, () => selectTenant(tenant)) )} </ul> </li> )

The subtenant list is a sub-list of the org tenant list, and so you can wrap that ul completely in the li

@arelia yeah, using li was my first approach but there is some default hover state and styling set to list items so I ended up creating a class to offset it called .organization-tenants-list but then I decided that a UL would be easier, in fact I should probably remove the wrapping <ul className="demo-list-icon mdl-list"> and have the resulting HTML output like <ul>org 1 list</ul><ul>org 2 list</ul><ul>org 3 list</ul>

I think semantically it makes sense to have it all wrapped in <ul className="demo-list-icon mdl-list"> but again, since we'll re-do all of this do whatever works and is easiest, that's fine with me! So if you don't make any more changes, I'm okay with that

…l legacy concerns (API, Localizations, Doorkeeper, Abilities, etc)

…enancy

…nt stylesheet

…amelCase

…ancyLookup to tenantLookupTable

…thDB

…ant FK to correct table, start to integrate with Tenant Switcher component

…nant

…enancy

…tenant list header

…enancy

…Tenant

… to base record class, simplify and correct various associations

…and Tenant

…ble ContentItems per-tenant, implement basic Org logic, simplify active_tenant behavior, refresh upon tenant switch, prepare seeds

CE-154: Tenancy

CE-188: Tenancy Pagination Logic & UI

MKwenhua added 3 commits August 29, 2017 14:43

feat: updated React on Rails and created React file structure

3d6c46a

feat(app/views/partials/_sidebar.html.haml): add react component

a84bca6

feat: create components

2e9a3de

MKwenhua added the don't merge label Sep 1, 2017

MKwenhua added 5 commits September 1, 2017 13:06

feat(components/sidebar.scss): create sidebar--tentant-display class …

f743f46

…for displaying tenant list

feat(layouts/application.html.haml): create wrapper div #layout_wrapp…

f9eff19

…er to hold sidenav classes

feat(models/user.rb): create new column active_tenant

7b67b91

feat(client/app/bundles/dashboard/containers/layout.jsx): use ReactOn…

7238299

…Rails.authenticityToken() api to obtain CSRF token

feat(dashboard/services/rails_api_service.jsx): create RailsAPIservic…

ab9b301

…e object

toastercup reviewed Sep 5, 2017

View reviewed changes

MKwenhua added 2 commits September 5, 2017 14:37

feat(containers/tenant_switcher_container.jsx): set TenantSwitcherCon…

eb34ded

…tainer to remove sidebar--tenant-display class from #layout_wrapper on successful tenant switch

feat(dashboard/helpers/tenant_organization_lookup.jsx): create helper…

912c34a

… method to index and create a lookup object for tenants by organization

MKwenhua mentioned this pull request Sep 5, 2017

COR-680: Tenant Switcher cortex-cms/cortex-plugins-core#60

Merged

MKwenhua added 5 commits September 5, 2017 16:38

feat(components/side_bar/tenant_list.jsx): add accessibility attributes

a1b23b5

refactor: remove commented out code

4410c9c

feat(db/seeds.rb): create tenants in seeds.rb

b540533

fix(dashboard/reducers/tentant_switcher_reducer.jsx): assign active_t…

2841f7f

…enant to tenant if null

feat(dashboard/reducers/tentant_switcher_reducer.jsx): set organizati…

f799dcb

…on_displayed back to selected_tenant organization

MKwenhua added enhancement infrastructure performance ui/ux and removed don't merge labels Sep 5, 2017

MKwenhua requested review from toastercup, arelia and mrfantasticwonders September 5, 2017 22:34

Merge branch 'develop' into COR-680/Tenant-Switcher

9aaa282

arelia approved these changes Sep 6, 2017

View reviewed changes

toastercup and others added 28 commits September 28, 2017 15:52

refactor(Migrations + Legacy): further simplify migrations, remove al…

956a5f2

…l legacy concerns (API, Localizations, Doorkeeper, Abilities, etc)

refactor(Migrations): finalize simplification

be4ffc6

feat(Seeds): example tenant + multiple tenants for initial user

515eabc

Merge branch 'CE-188/Tenancy-Pagination-Logic/UI' into topic/CE-154-T…

d16ac96

…enancy

refactor(Tenant): add has_many to other side of active_tenant relation

d3cde61

feat(components/tenant_switcher.scss): create tenant_switcher compone…

4882beb

…nt stylesheet

refactor(client/package.json): remove devDependencies modules

bb444a4

refatcor: change JS attributes selected_tenant and parent_tenant to c…

c72e961

…amelCase

refactor(app/bundles/cortex/*): change tenancy loop up table from ten…

bc94b71

…ancyLookup to tenantLookupTable

refactor(app/bundles/cortex/*): change syncedWithDB to tenantSyncedWi…

c5cdc21

…thDB

feat(Tenancy): add example subtenant to initial seed, move active_ten…

0dcdecd

…ant FK to correct table, start to integrate with Tenant Switcher component

feat(cortex/reducers/tentant_switcher_reducer.jsx): pull in active_te…

56a3683

…nant

Merge branch 'CE-188/Tenancy-Pagination-Logic/UI' into topic/CE-154-T…

b26058d

…enancy

fix(cortex/components/side_bar/tenant_list.jsx): remove double tag

40e1b77

Merge branch 'CE-188/Tenancy-Pagination-Logic/UI' into topic/CE-154-T…

f72ccfd

…enancy

refactor(components/tenant_switcher.scss): fix styles

4ff48be

refactor(components/tenant_switcher.scss): remove border bottom from …

2f9d626

…tenant list header

feat(Tenant Switcher): implement tenants_with_children and pass as props

b975bea

Merge branch 'CE-188/Tenancy-Pagination-Logic/UI' into topic/CE-154-T…

d206a4d

…enancy

refactor(Tenant Switcher): condense, simplify logic/naming for active…

1467da5

…Tenant

feat(Tenancy): BelongsToTenant abstraction, abstract acts_as_paranoid…

4f48f8b

… to base record class, simplify and correct various associations

refactor(Tenancy): remove direct association between Field/FieldItem …

147696f

…and Tenant

feat(Tenancy): scope available ContentTypes across Orgs, scope availa…

fc5c0da

…ble ContentItems per-tenant, implement basic Org logic, simplify active_tenant behavior, refresh upon tenant switch, prepare seeds

chore(Gemfile): bump cortex-plugins-core + various gems

511fdfa

feat(Tenancy): add additional presence validations, associations

1f5f8e4

Merge pull request #527 from cbdr/topic/CE-154-Tenancy

5b09267

CE-154: Tenancy

chore(plugins-core): bump to v1.1.0

297c2c2

Merge pull request #526 from cbdr/CE-188/Tenancy-Pagination-Logic/UI

b2369d9

CE-188: Tenancy Pagination Logic & UI

toastercup merged commit 33548c8 into develop Oct 4, 2017

toastercup deleted the COR-680/Tenant-Switcher branch October 4, 2017 22:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

COR-680: Tenant Switcher #522

COR-680: Tenant Switcher #522

MKwenhua commented Sep 1, 2017 •

edited

Loading

toastercup Sep 5, 2017

MKwenhua Sep 5, 2017

arelia Sep 6, 2017

MKwenhua Sep 6, 2017

toastercup commented Sep 6, 2017

MKwenhua commented Sep 6, 2017

arelia left a comment

arelia Sep 6, 2017

MKwenhua Sep 6, 2017

arelia Sep 6, 2017

arelia Sep 6, 2017

MKwenhua Sep 6, 2017

arelia Sep 6, 2017

arelia Sep 6, 2017

MKwenhua Sep 6, 2017

arelia Sep 6, 2017

MKwenhua Sep 6, 2017

arelia Sep 6, 2017

arelia Sep 6, 2017

arelia Sep 6, 2017

MKwenhua Sep 6, 2017

arelia Sep 6, 2017

MKwenhua Sep 6, 2017

arelia Sep 6, 2017

COR-680: Tenant Switcher #522

COR-680: Tenant Switcher #522

Conversation

MKwenhua commented Sep 1, 2017 • edited Loading

Purpose:

JIRA:

Steps to Take On Prod

Changes:

Screenshots

QA Links:

How to Verify These Changes

Relevant PRs/Dependencies:

Additional Information

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

toastercup commented Sep 6, 2017

MKwenhua commented Sep 6, 2017

arelia left a 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

MKwenhua commented Sep 1, 2017 •

edited

Loading