Skip to content
This repository was archived by the owner on Oct 11, 2023. It is now read-only.

GoToTicket box is shown on all pages#131

Open
pdimitratos wants to merge 8 commits into
Azure:masterfrom
pdimitratos:ux/GoToTicket
Open

GoToTicket box is shown on all pages#131
pdimitratos wants to merge 8 commits into
Azure:masterfrom
pdimitratos:ux/GoToTicket

Conversation

@pdimitratos

Copy link
Copy Markdown
Contributor

For Task 47549: Replace "Incident Search" page with "Go to Ticket" field.

This is extremely unlikely to be the style/layout we want, but is this the correct functionality? GoToTicket is shown on all pages.

@pdimitratos pdimitratos requested a review from alrodrig1 April 10, 2018 19:32
@codecov-io

codecov-io commented Apr 10, 2018

Copy link
Copy Markdown

Codecov Report

Merging #131 into master will decrease coverage by 0.1%.
The diff coverage is 73.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #131      +/-   ##
==========================================
- Coverage   61.66%   61.56%   -0.11%     
==========================================
  Files          95       95              
  Lines        1414     1418       +4     
==========================================
+ Hits          872      873       +1     
- Misses        542      545       +3
Impacted Files Coverage Δ
src/components/MainComponent.js 0% <ø> (ø) ⬆️
src/components/Incident/Ticket.js 0% <0%> (ø) ⬆️
src/components/Home.js 100% <100%> (ø) ⬆️
src/actions/incidentActions.js 67.39% <100%> (ø) ⬆️
src/components/Incident/GoToTicket.js 90% <87.5%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c4d5dc...4ef5499. Read the comment docs.

@coveralls

coveralls commented Apr 10, 2018

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.1%) to 57.206% when pulling 4ef5499 on pdimitratos:ux/GoToTicket into 8c4d5dc on Azure:master.

@alrodrig1

Copy link
Copy Markdown
Member

For some reason this doesn't run on my machine:

image

@alrodrig1 alrodrig1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Managed to build and run on my local. This seems to be the correct functionality.

Regarding layout, something like this probably works:
image

Also, make sure to remove the search page, and remove it from the menu as well.

@danieljurek danieljurek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one comment worth consideration: incidentActions.js:147

The others don't require the action specified or require action in general.

}

export const updateIncidentCreationInput = (input) => ({
export const updateTicketNavigationInput = (input, history) => ({

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

history appears to go unused in any calls to updateTicketNavigationInput and within the function body itself. Is it necessary?

})
})

describe('GoToTicket render output', function () {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to change anything here... Taking advantage of another soapbox opportunity

These tests are manually verifying aspects of the component but they're not handling scenarios where, for example, a child could be added after result.props.children[1]

export const ConnectedGoToTicketForm = connect(mapStateToProps)(GoToTicketForm)

export const GoToTicket = ({history}) => <Paper zDepth={1}>
<span>Go To Ticket:</span>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[This will almost certainly be less relevant given our discussion at the standup]

The <form> tag results in a line break... which makes the form harder to understand and creates an opportunity to evangelize styles.

To get "Go To Ticket" on the same line as the form:

// After the includes, near the top of the file: 
const styles = {
  inlineForm: { display: 'inline', padding: '16px' }
}
...
export const GoToTicketForm = ({input, creationError, history, dispatch}) => <form
  id='incident-navigation'
  onSubmit={onSubmit(input, history)}
  style={styles.inlineForm}      // <-- Access the styles object to apply the styles to the element
>
...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From our conversation earlier and the wireframe up top, Toolbars might give us what we need here: http://www.material-ui.com/#/components/toolbar

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants