TP: adds the ability to rearrange table columns as well as toggle column visibility by mitchell852 · Pull Request #3680 · apache/trafficcontrol

mitchell852 · 2019-06-14T21:51:42Z

This PR adds the ability to show/hide table columns in TP as well as reorder the visible columns of a table. The changes made by the user will be persisted in the browsers local storage.

I tackled an easy table (the types table) to start and prove out the concept.

This PR is not related to any Issue

Which Traffic Control components are affected by this PR?

Traffic Portal

What is the best way to verify this PR?

Run the UI tests
Manually verify

Launch TP
navigate to Configure > Types
reorder columns via drag/drop on the column header
show/hide columns by using the following drop down

Note: I'm not going to document this functionality or add it to the changelog until it is added to other tables in subsequent PRs.

The following criteria are ALL met by this PR

This PR includes tests OR I have explained why tests are unnecessary
This PR includes documentation OR I have explained why documentation is unnecessary
This PR includes an update to CHANGELOG.md OR such an update is not necessary
This PR includes any and all required license headers
This PR ensures that database migration sequence is correct OR this PR does not include a database migration
This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

asfgit · 2019-06-14T22:00:19Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3860/
Test FAILed.

asfgit · 2019-06-14T22:19:41Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3861/
Test FAILed.

asfgit · 2019-06-18T20:47:13Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3867/
Test FAILed.

asfgit · 2019-06-18T20:54:16Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3868/
Test FAILed.

asfgit · 2019-06-18T21:21:52Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3869/
Test FAILed.

asfgit · 2019-06-19T02:23:23Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3870/
Test FAILed.

asfgit · 2019-06-19T15:15:42Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3871/
Test FAILed.

asfgit · 2019-06-19T15:45:17Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3872/
Test FAILed.

asfgit · 2019-06-19T16:14:48Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3873/
Test FAILed.

ocket8888

Still need to test things, will review again after testing.

ocket8888 · 2019-06-19T18:15:24Z

traffic_portal/app/src/common/modules/table/types/TableTypesController.js

The standard Storage interface doesn't appear to support indexing. It does work in Firefox, and I assume you tested it in Chrome, so maybe we don't care, but indexing seems to be non-standard and implementation-dependent. I'd recommend using Storage.getItem() instead.

This line also has an uncaught SyntaxError from that call to JSON.parse, which I guess is fine if it's somehow implicitly handled by the dataTables? Seems unlikely though.

You're also accessing a property of an object by indexing it with a string, which isn't exactly a problem in this case because of how the Storage API works, but it's a bad habit to get into because it forces type coercion on the indexed value.

Finally, you should check for the availability of the localStorage object (MDN Guide). This is because some browsers don't have localStorage (though I'm sure we don't support them), but also because sometimes it may just not be available even if it is supported. As an example, Safari's private browsing mode gives you an empty object with 0 quota on every access. It's possible that dataTablesSort or whatever it's called does this already, but you should be sure; I'm not.

ok, i'll see what i can clean up here. thanks

ocket8888 · 2019-06-19T18:16:43Z

traffic_portal/app/src/common/modules/table/types/table.types.tpl.html

I hate this name, because it's tying our UI directly to object representation in the database, which is meant to be opaque to Traffic Portal. But I don't have a better suggestion, so if you don't either, I guess it's fine.

yes, i'm not a fan either. it's a bad practice imo that bubbled up to the UI

ocket8888 · 2019-06-19T18:20:27Z

traffic_portal/app/src/common/modules/table/types/table.types.tpl.html

a [role="menuitem"] must be the direct child of a [role="menu"] or [role="menubar"] - you can add these roles to the <ul>, or you can get that implicitly by using a <menu> instead.

asfgit · 2019-06-19T19:29:51Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3874/
Test FAILed.

asfgit · 2019-06-19T19:54:22Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3875/
Test FAILed.

asfgit · 2019-06-19T19:59:22Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3876/
Test FAILed.

ocket8888

Dude you gotta minify that new library - it's more than 10 times bigger than the previous version

ocket8888 · 2019-06-19T22:34:29Z

traffic_portal/app/src/app.js

wasn't this important?

no. i thought it might be useful but i never used it. there's an init function in applicationService you can use instead if you need to do something at startup.

👍 that sounds like a more proper use of Angular anyway.

asfgit · 2019-06-19T22:40:10Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3878/
Test FAILed.

asfgit · 2019-06-19T22:44:59Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3879/
Test FAILed.

asfgit · 2019-06-25T18:50:38Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3907/
Test PASSed.

asfgit · 2019-06-25T19:57:52Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3909/
Test FAILed.

asfgit · 2019-06-25T20:15:56Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3911/
Test PASSed.

ocket8888

Tests pass, manually tested the table changes, code looks good.

…visiblity/searchability - started with types table

leveraged new col.name property to enable ng-repeat on column checkboxes

…y when visible is changed.

…ges can be captured in history via a seperate PR

…ifications that were made

…00% width

…umn checkboxes

asfgit · 2019-07-18T15:16:47Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3984/
Test PASSed.

mitchell852 added new feature A new feature, capability or behavior Traffic Portal v1 related to Traffic Portal version 1 labels Jun 14, 2019

mitchell852 force-pushed the column-config branch 2 times, most recently from a8f1be4 to 4fda031 Compare June 14, 2019 21:54

mitchell852 force-pushed the column-config branch from 4fda031 to 1aa70cf Compare June 14, 2019 22:12

mitchell852 force-pushed the column-config branch 3 times, most recently from e882b33 to 6c3067a Compare June 18, 2019 20:46

mitchell852 force-pushed the column-config branch from 6c3067a to 8e66460 Compare June 18, 2019 20:55

mitchell852 force-pushed the column-config branch from 8e66460 to d5e3621 Compare June 19, 2019 01:50

mitchell852 marked this pull request as ready for review June 19, 2019 16:20

mitchell852 changed the title ~~WIP - TP adds the ability to rearrange table columns as well as toggle column visibility…~~ TP adds the ability to rearrange table columns as well as toggle column visibility/searchability Jun 19, 2019

mitchell852 changed the title ~~TP adds the ability to rearrange table columns as well as toggle column visibility/searchability~~ TP: adds the ability to rearrange table columns as well as toggle column visibility/searchability Jun 19, 2019

ocket8888 suggested changes Jun 19, 2019

View reviewed changes

mitchell852 force-pushed the column-config branch from 4cae9cf to 8a6e531 Compare June 19, 2019 19:54

ocket8888 suggested changes Jun 19, 2019

View reviewed changes

ocket8888 approved these changes Jun 25, 2019

View reviewed changes

mitchell852 added 18 commits July 2, 2019 08:05

adds the ability to rearrange table columns as well as toggle column …

4d31f45

…visiblity/searchability - started with types table

updates jquery datatables to version 1.10.19

8de8ad5

adds column name to saved state

7fb2969

removed jquery click event handler in favor of angular approach

2b15729

leveraged new col.name property to enable ng-repeat on column checkboxes

reordered default columns

30bfb1b

added logic to the jquery datatables to handle the searchable propert…

537ba00

…y when visible is changed.

renamed the jquery.datatables library and added proper licenses

870315c

uses localstorage api

3d8e926

checks for local storage and removes an unused startup method

125af47

using menu tag instead of ul/li

b81f4c0

fixes license for colReorder.datatables

0e8a7be

provides an informative console message if localstorage parse fails

36e87c2

reverts to the the unpatched version of jquery datatables so any chan…

c31c441

…ges can be captured in history via a seperate PR

reminified jquery datatables js file and added comments about the mod…

e85981f

…ifications that were made

menuitem tag was deprecated

16a975d

ensures that showing/hiding table columns always keeps the table at 1…

91ce4a0

…00% width

updated the default column order and used an orderby to order the col…

de58a92

…umn checkboxes

removes name attribute from tds

62174d9

mitchell852 mentioned this pull request Jul 15, 2019

CLOSED - TP: adds the ability to rearrange delivery service table columns as well as toggle column visibility/searchability #3722

Closed

7 tasks

mitchell852 assigned mitchell852 and unassigned mitchell852 Jul 17, 2019

adds a ui test to toggle the visiblity of the table columns

90b659d

mitchell852 force-pushed the column-config branch from c7c4dff to 90b659d Compare July 18, 2019 15:10

rawlinp approved these changes Jul 18, 2019

View reviewed changes

rawlinp merged commit 487efdc into apache:master Jul 18, 2019

Conversation

mitchell852 commented Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which Traffic Control components are affected by this PR?

What is the best way to verify this PR?

The following criteria are ALL met by this PR

Uh oh!

asfgit commented Jun 14, 2019

Uh oh!

asfgit commented Jun 14, 2019

Uh oh!

asfgit commented Jun 18, 2019

Uh oh!

asfgit commented Jun 18, 2019

Uh oh!

asfgit commented Jun 18, 2019

Uh oh!

asfgit commented Jun 19, 2019

Uh oh!

asfgit commented Jun 19, 2019

Uh oh!

asfgit commented Jun 19, 2019

Uh oh!

asfgit commented Jun 19, 2019

Uh oh!

ocket8888 left a comment

Choose a reason for hiding this comment

Uh oh!

ocket8888 Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

mitchell852 Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

ocket8888 Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitchell852 Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

ocket8888 Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfgit commented Jun 19, 2019

Uh oh!

asfgit commented Jun 19, 2019

Uh oh!

asfgit commented Jun 19, 2019

Uh oh!

ocket8888 left a comment

Choose a reason for hiding this comment

Uh oh!

ocket8888 Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

mitchell852 Jun 20, 2019

Choose a reason for hiding this comment

Uh oh!

ocket8888 Jun 20, 2019

Choose a reason for hiding this comment

Uh oh!

asfgit commented Jun 19, 2019

Uh oh!

asfgit commented Jun 19, 2019

Uh oh!

asfgit commented Jun 25, 2019

Uh oh!

asfgit commented Jun 25, 2019

Uh oh!

asfgit commented Jun 25, 2019

Uh oh!

ocket8888 left a comment

Choose a reason for hiding this comment

Uh oh!

asfgit commented Jul 18, 2019

Uh oh!

Reviewers

Assignees

mitchell852 commented Jun 14, 2019 •

edited

Loading

ocket8888 Jun 19, 2019 •

edited

Loading

ocket8888 Jun 19, 2019 •

edited

Loading