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

Add link to volumes to docker settings #660

Merged
merged 5 commits into from
Mar 4, 2016

Conversation

Poltergeist
Copy link
Contributor

This adds a link from the docker section to the volumes section. This may be removed at a later time. @leemunroe thought it would be a good idea to have this for usability purposes.

@leemunroe
Copy link
Contributor

👍

Mainly for any existing users who are confused about where the settings have went.

@@ -27,10 +28,16 @@ var CollapsiblePanelComponent = React.createClass({
this.setState({
isOpen: nextProps.isOpen
});
if (this.props.togglePanel != null) {
this.props.togglePanel(false);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Small thing, but the presence of this test seems rather unidiomatic to me - I'd prefer props.togglePanel to be required or no-op by default.

@pierluigi
Copy link
Contributor

Would be nice to have a gif/screenshot for UI changes in PRs ;)

isOpen={
this.fieldsHaveError({volumes: "volumes"}) ||
state.isVolumesOpen
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we extract this rather than having it all inline? Also I'd prefer to avoid the OR if possible. I think both of these are in our style guide.

@philipnrmn
Copy link
Contributor

Not sure I like the wording You can set your Docker volume settings below, it's a little bit doppelt gemoppelt. Can we have use change or update instead of set?

@Poltergeist
Copy link
Contributor Author

You are right probably You can set your Docker volume settings below is better.

@pierluigi
Copy link
Contributor

@Poltergeist the problem is set / settings. How about You can configure your Docker volume settings below?

@philipnrmn
Copy link
Contributor

^^ Agreed. Configure is a nicer verb - we could also have You can configure your Docker volume below

@Poltergeist
Copy link
Contributor Author

s/set/configure/g :-)

@philipnrmn
Copy link
Contributor

Sorry for the multiple rounds of review, I didn't test manually the first time and got bitten by the omission.

Having done so, I see a couple of issues

  1. It's very hard to tell that below is a link. We should either distinguish it somehow or link the whole text.
  2. The cursor remains a text cursor when the user hovers the link. We should enforce cursor:pointer or set an href.

@philipnrmn
Copy link
Contributor

cc @leemunroe

@Poltergeist Poltergeist force-pushed the feature/link-volumes-from-docker branch from a44afce to 54eba54 Compare March 4, 2016 11:16
philipnrmn added a commit that referenced this pull request Mar 4, 2016
@philipnrmn philipnrmn merged commit 29648b8 into master Mar 4, 2016
@philipnrmn philipnrmn deleted the feature/link-volumes-from-docker branch March 4, 2016 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants