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

Refactor right panel header buttons #5117

Merged
merged 5 commits into from Sep 25, 2017

Conversation

@lukebarnard1
Copy link
Contributor

lukebarnard1 commented Sep 25, 2017

To reduce repetition of code, simplify the process of adding header buttons and remove the need for many handler functions.

To reduce repetition of code, simplify the process of adding header buttons and remove the need for many handler functions.
@lukebarnard1

This comment has been minimized.

Copy link
Contributor Author

lukebarnard1 commented Sep 25, 2017

(I'll squash merge this)

Copy link
Member

dbkr left a comment

otherwise lgtm

// If currentPhase is one of the specified phases, the button will be highlighted
phases: PropTypes.arrayOf(PropTypes.string).isRequired,
// The currentPhase of the RightPanel
currentPhase: PropTypes.string.isRequired,

This comment has been minimized.

Copy link
@dbkr

dbkr Sep 25, 2017

Member

Is there likely to be any need for the button to know the actual phases it represents or what the current phase is? Could we just pass in whether it should be highlighted or not?

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Sep 25, 2017

Author Contributor

Could we just pass in whether it should be highlighted or not?

I was hoping to abstract this logic away such that, for example, we can't get two buttons highlighted at once. It's probably overkill but if every single header button is going to embody this logic, it seems inefficient to dupe it for each one.

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Sep 25, 2017

Author Contributor

Also, it needs to know for the dispatch so 🤷‍♂

@dbkr dbkr assigned lukebarnard1 and unassigned dbkr Sep 25, 2017
@lukebarnard1 lukebarnard1 assigned dbkr and unassigned lukebarnard1 Sep 25, 2017
Use isHighlighted as a prop instead of computing based on phases and currentPhase
@dbkr dbkr merged commit 798558e into develop Sep 25, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.