Task Search UI #890

Merged
merged 97 commits into from Mar 14, 2016

Conversation

Projects
None yet
5 participants
@Calvinp
Contributor

Calvinp commented Feb 9, 2016

This Implements the UI for the new global and request task search APIs.
It is written in React, which has only been used for one other Singularity component (the aggregate log tailer).
It allows viewing up to 50 tasks per page, clearing search parameters, modifying search order, paging through search options, and returning to request.
It also includes some components, in a 'common' folder, that may be useful for others using react later on.

Calvinp added some commits Jan 28, 2016

Drop downs and form fields are their own elements, but they have stat…
…e that maybe should be held by the TaskSearch page instead
Now the results page actually loads, and doesn't instantly revert bac…
…k to the form page. Yay! Also some code organizational changes.
Now the results page actually loads, and doesn't instantly revert bac…
…k to the form page. Yay! Also some code organizational changes. And added files that weren't in the previous commit by accident
+ base += 'page=' + @page
+ anyParams = true
+ return '' if not anyParams
+ return base

This comment has been minimized.

@ssalinas

ssalinas Feb 9, 2016

Member

We can build the params a bit cleaner here. Probably easier to build up an array of params (i.e. if @Attribute then add to array), then return paramsArray.join('&')

@ssalinas

ssalinas Feb 9, 2016

Member

We can build the params a bit cleaner here. Probably easier to build up an array of params (i.e. if @Attribute then add to array), then return paramsArray.join('&')

This comment has been minimized.

@tpetr

tpetr Feb 9, 2016

Member

an even better solution would be to build an Object of all the query params to set, and then use $.param() to convert the object into a legit query string

@tpetr

tpetr Feb 9, 2016

Member

an even better solution would be to build an Object of all the query params to set, and then use $.param() to convert the object into a legit query string

This comment has been minimized.

@ssalinas

ssalinas Feb 9, 2016

Member

TIL... thanks @tpetr 😃

@ssalinas

ssalinas Feb 9, 2016

Member

TIL... thanks @tpetr 😃

+
+ model: TaskSearchResult
+
+ params: ->

This comment has been minimized.

@ssalinas

ssalinas Feb 9, 2016

Member

because a number of our backbone collections and other classes have an @params of some sort, it might be easier to rename this method to something like queryParams or getQueryParams

@ssalinas

ssalinas Feb 9, 2016

Member

because a number of our backbone collections and other classes have an @params of some sort, it might be easier to rename this method to something like queryParams or getQueryParams

+ base = "#{ config.apiRoot }/history/request/#{ @requestId }/tasks"
+ else
+ base = "#{ config.apiRoot }/history/tasks"
+ return base + @params()

This comment has been minimized.

@ssalinas

ssalinas Feb 9, 2016

Member

You can do this with string interpolation rather than adding strings, e.g.

if @requestId
    "#{ config.apiRoot }/history/request/#{ @requestId }/tasks?#{ @queryParams() }"
else
    "#{ config.apiRoot }/history/tasks?#{ @queryParams() }"
@ssalinas

ssalinas Feb 9, 2016

Member

You can do this with string interpolation rather than adding strings, e.g.

if @requestId
    "#{ config.apiRoot }/history/request/#{ @requestId }/tasks?#{ @queryParams() }"
else
    "#{ config.apiRoot }/history/tasks?#{ @queryParams() }"
+ anyParams = false
+ base = '?'
+ if @deployId
+ base += 'deployId=' + @deployId

This comment has been minimized.

@ssalinas

ssalinas Feb 9, 2016

Member

Let's make sure these are all spaces not tabs, it's burned us earlier when testing this out. Comment applies for all files

@ssalinas

ssalinas Feb 9, 2016

Member

Let's make sure these are all spaces not tabs, it's burned us earlier when testing this out. Comment applies for all files

+Link = React.createClass
+
+ getLink: ->
+ return window.config.appRoot + "/task/" + @props.taskId

This comment has been minimized.

@ssalinas

ssalinas Feb 9, 2016

Member

can just use config here, don't need it to be window.config

also, another case for the string interpolation here

"#{ config.apiRoot }/task/#{ @props.taskId }"
@ssalinas

ssalinas Feb 9, 2016

Member

can just use config here, don't need it to be window.config

also, another case for the string interpolation here

"#{ config.apiRoot }/task/#{ @props.taskId }"

This comment has been minimized.

@Calvinp

Calvinp Feb 9, 2016

Contributor

Actually, that function isn't even used - it's leftover from when I was doing this a different way, and I guess I forgot to delete it.

@Calvinp

Calvinp Feb 9, 2016

Contributor

Actually, that function isn't even used - it's leftover from when I was doing this a different way, and I guess I forgot to delete it.

+
+ handleSubmit: (event) ->
+ event.preventDefault()
+ @setState({

This comment has been minimized.

@ssalinas

ssalinas Feb 9, 2016

Member

more of a space-saving/readability thing we can do with coffeescript

@setState showForm: false

is the same as

 @setState({
      showForm: false
})
@ssalinas

ssalinas Feb 9, 2016

Member

more of a space-saving/readability thing we can do with coffeescript

@setState showForm: false

is the same as

 @setState({
      showForm: false
})
+Controller = require './Controller'
+
+TaskSearchView = require '../views/taskSearch'
+TaskSearchResults = require '../collections/TaskSearchResults'

This comment has been minimized.

@ssalinas

ssalinas Feb 9, 2016

Member

Doesn't look like this is used in the controller here

@ssalinas

ssalinas Feb 9, 2016

Member

Doesn't look like this is used in the controller here

@@ -0,0 +1,63 @@
+Collection = require './collection'

This comment has been minimized.

@ssalinas

ssalinas Feb 10, 2016

Member

Looking at our other collections/models, it seems we are doing some double work with this and the RequestHistoricalTasks collection. Both are the same url, this one has the extra params, the other has a few more methods. I would argue that we merge the two together (maybe into RequestHistoricalTasks for naming). RequestHistoricalTasks also already has a TaskHistoryItem defined that we can use in place of the TaskSearchResult model

@ssalinas

ssalinas Feb 10, 2016

Member

Looking at our other collections/models, it seems we are doing some double work with this and the RequestHistoricalTasks collection. Both are the same url, this one has the extra params, the other has a few more methods. I would argue that we merge the two together (maybe into RequestHistoricalTasks for naming). RequestHistoricalTasks also already has a TaskHistoryItem defined that we can use in place of the TaskSearchResult model

tpetr and others added some commits Feb 25, 2016

@ssalinas ssalinas added the hs_qa label Feb 26, 2016

Calvinp and others added some commits Feb 29, 2016

Refactor RequestHistoricalTasks, DeployHistoricalTasks and TaskSearch…
…Results collections into HistoricalTasks
Merge pull request #937 from HubSpot/merge-task-history-collections
Refactor HistoricalTasks Collections into One

ssalinas added a commit that referenced this pull request Mar 14, 2016

@ssalinas ssalinas merged commit 31ca810 into task_search Mar 14, 2016

0 of 2 checks passed

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

@ssalinas ssalinas deleted the search-tasks-react branch Mar 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment