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

Sort #395

Merged
merged 33 commits into from Aug 16, 2016

Conversation

Projects
None yet
3 participants
@mattwchun
Copy link
Contributor

mattwchun commented Aug 3, 2016

Description of the problem this pull request fixes
This feature allows users to sort based on rows in the DataTable. This will be done with vega sort data transform code added to the datatable spec.

Changes proposed in this pull request:

- sorting feature to allows users to sort based on rows in the DataTable.

** If submitting code for review: **

This PR includes:

  • Tests
  • functional comments
  • high level description of any new components

Steps to functionally test this:

mattwchun added some commits Jul 15, 2016

Added icon for sorting in data table. May need a new icon since has a…
…uthor's name in the svg icon. Next created component but component has some binding warnings and proptype errors
still getting error: warning.js:45 Warning: Failed propType: Invalid …
…prop of type supplied to , expected . Check the render method of .
deleted extra decreasingSort svg and editted with comments the decrea…
…singSort.svg in assets to be styled better in datatable
started working on function in export.js that would take new state an…
…d edit the vega spec with the added transform to sort the values
Added an getSort() function that takes a dataset. This function const…
…ructs the transform part of the vega spec that will be returned in export.dataset(). This function is meant to be used as a helper function for export.dataset(). This is ultimately used to add the sort feature to the DataTable

@arvind arvind added the in progress label Aug 3, 2016

mattwchun added some commits Aug 4, 2016

Added another action to the vegaReducer invalidating actions to spawn…
… a refresh of the dataset after add transform to the spec. Sort feature seems to be working but will need to test now
changed the increasingSort and decreasingSort svg icons. These icons …
…are toggled when click on sort. Found a bug. When click on the icon, only after the hoverField is unhovered the DataTable is updated.
@AnjirHossain

This comment has been minimized.

Copy link
Member

AnjirHossain commented Aug 5, 2016

Just went through the QA testing flow, the pr didn't introduce any regressions and all the expected sorting functionality is present. 1+ functional!

mattwchun added some commits Aug 5, 2016

merged with updated lyra2 branch. Working sort feature just need to e…
…xtra out the changing sort icon into its own component to modularize the code better
Added some simple tests for datasetReducer. Tests include one that te…
…sts to see if datasetReducer is a function, returns the original state if not an action it accepts, and takes an SORT_DATASET action and updates the store correctly
Created test file to test datasetActions action creator method sortDa…
…taset. Tested that all the properties are set in the action returned from sortDataset and the action creator returns an object
added header comments to the getSort() function in export.js that cre…
…ates the vega data transform code to be appended to to the spec returned in exporter dataset that will sort the dataset.
Added a test to test the correct data transform spec is added to the …
…vega spec returned from exporter dataset. This therefore tests the getSort() function added to exporter which is called in exporter dataset.
Renamed the tests for exporter dataset. Wrote 2 total new tests which…
… consist of an increasing and decreasing sort. This is to test if the correct data transform vega code is added to the spec returned in exporter dataset
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:sketch="http://www.bohemiancoding.com/sketch/ns" viewBox="0 0 100 100" version="1.1" x="0px" y="0px"><title>Sort Ascending</title><desc>Created with Sketch.</desc><g stroke="none" stroke-width="1" fill="none" fill-rule="evenodd" sketch:type="MSPage"><g sketch:type="MSArtboardGroup" fill="#000000"><g sketch:type="MSLayerGroup" transform="translate(8.000000, 4.000000)"><path d="M64,79 L56.7191654,87.0922045 L64.2221013,93.7032523 L78.7679977,77.1949874 L86.0708143,69 L75.0165311,69 L0,69 L0,79 L64,79 Z" sketch:type="MSShapeGroup"/><path d="M86,0 L76,0 L76,63 L86,63 L86,0 Z" sketch:type="MSShapeGroup"/><path d="M67,63 L67,13 L57,13 L57,63 L67,63 L67,63 Z" sketch:type="MSShapeGroup"/><path d="M48,63 L48,25 L38,25 L38,63 L48,63 L48,63 Z" sketch:type="MSShapeGroup"/><path d="M29,63 L29,38 L19,38 L19,63 L29,63 L29,63 Z" sketch:type="MSShapeGroup"/><path d="M10,63 L10,50 L0,50 L0,63 L10,63 L10,63 Z" sketch:type="MSShapeGroup"/></g></g></g>
<!--<text x="0" y="115" fill="#000000" font-size="5px" font-weight="bold" font-family="'Helvetica Neue', Helvetica, Arial-Unicode, Arial, Sans-serif">Created by Chris</text><text x="0" y="120" fill="#000000" font-size="5px" font-weight="bold" font-family="'Helvetica Neue', Helvetica, Arial-Unicode, Arial, Sans-serif">from the Noun Project</text>-->
<!-- changed the height in viewBox to 125 to 100 -->
</svg>

This comment has been minimized.

@arvind

arvind Aug 15, 2016

Member

These two icons are pretty difficult to identify in the HoverField. I've had more luck with the FontAwesome icon set. You can find SVGs for them here.

With regards to which icon to show (and I'm now referring to the corresponding FontAwesome icons here), let's start by showing sort (double arrow) to indicate that one can sort a field, but that it has not yet been sorted. Once a user clicks the icon, the icon should then switch to indicate the current sort order (i.e., should show ascending if ascending and descending if descending). Let's also use the icon most appropriate for the field type (i.e., sort-alpha- for categorical fields, and sort-numeric- for the other two).


field = field ? (
<div>
<FieldType field={field} />
{field.name}
{incOrDecIcon}

This comment has been minimized.

@arvind

arvind Aug 15, 2016

Member

Can you extract all the changes you've made to this component into a separate SortField.jsx component? The goal would be to create a fairly standalone/self-contained component, much like we did with FieldType.jsx. Unless I'm mistaken, the SortField component should not require any local state (i.e., this.state within the react component itself). Instead, we should be able to drive it entirely based on what is stored in the dataset._sort in the store.

if(sort == undefined) {
return spec;
} else {
spec['transform'] = sort;

This comment has been minimized.

@arvind

arvind Aug 15, 2016

Member

spec.transform will suffice here. Stylistically, we prefer to only use square bracket notation when the key is an invalid identifier (e.g., contains symbols).

* vega data transform code to be appended to the vega spec to * the dataset
*/
function getSort(dataset) {
if (!('_sort' in dataset)) {

This comment has been minimized.

@arvind

arvind Aug 15, 2016

Member

Simply if (!dataset._sort) should be fine here.

}
this.setState({ valuesInc : valuesInc });

incOrDec = valuesInc ? 'inc' : 'dec';

This comment has been minimized.

@arvind

arvind Aug 15, 2016

Member

Let's use asc or desc instead to conform to typically expected sort order values. (Be sure to update these throughout).

} else {
valuesInc = !valuesInc;
}
this.setState({ valuesInc : valuesInc });

This comment has been minimized.

@arvind

arvind Aug 15, 2016

Member

Be sure to run npm run lint to catch errors in code style like the additional spaces here.

@arvind

This comment has been minimized.

Copy link
Member

arvind commented Aug 15, 2016

Hi @mattwchun, this looks good for the most part. I've left you a handful of comments throughout, and will merge this once they're handled. Thanks!

mattwchun and others added some commits Aug 16, 2016

Deleted increasingSort.svg and decreasingSort.svg and added 4 new ico…
…ns from FontAwesome. Created new component for SortField. Sort feature not completely functional since SortField will sort values but when unhover the values get sorted back to what they were. Also fixed stylistic errors in export.js. Also need to still run npm run lit to test the style
Removed store.getState() and moved to mapStateToProps. Also removed I…
…con require in HoverField since moved SortField into its own component
Fixed more style issues in exporter tests. Fixed isSortAsc to check i…
…f sort.sortOrder property in store is asc or desc instead of checking if sort is asc or desc
Changed SortField so that when sort one field the icon changes to a s…
…pecific sort icon but the other rows don't show that they are sorted
Refine sort field functionality.
Simplify code paths in the SortField component, move hardcoded strings to
constant files, and make stored state more concise.
Only re-render DataTable once Vega is done parsing.
Previously, the sort action would trigger a DataTable re-render before Vega
was done parsing causing the dataset's *input* values being temporarily used.

@arvind arvind merged commit 84af8c4 into lyra2 Aug 16, 2016

1 check was pending

continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@arvind arvind removed the in progress label Aug 16, 2016

@arvind arvind deleted the Sort branch Aug 16, 2016

@arvind

This comment has been minimized.

Copy link
Member

arvind commented Aug 16, 2016

Hi @mattwchun, great work overall! I would take a look at bf80429 to see the ways in which I iterated on and refined your work. In particular, I extracted any hardcoded strings that were reused in multiple places into a standalone constants file.

I also simplified the code path in the SortField component. This was motivated partly by an error-case I observed. Previously, if no fields were sorted and I clicked to sort fieldA it would be sorted in ascending order. Then, if I clicked to sort fieldB, that would flip to descending order on the first click. This is because you were considering the sort state to flip the order, rather than looking at if the fields matched.

Finally, some stylistic changes -- e.g., preferring a more concise store shape (._sort.field and ._sort.order rather than ._sort.sortField and ._sort.sortOrder).

Hope that helps guide your future PRs! Let me know if you have any questions. Great work overall!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.