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

Updating the bullet chart design pattern #697

Merged
merged 3 commits into from
Aug 3, 2018

Conversation

bmignano
Copy link
Contributor

@bmignano bmignano commented Jul 19, 2018

Description

Updating the bullet chart design pattern to match changes needed due to pattern implementation

JIRA: https://patternfly.atlassian.net/browse/OSUX-665

Closes: #662

Changes

  • Adding Measure Greater Than Max Range to Design page
  • Editing Variations callout image on Overview page

Contribution Submission Checklist

  • A GitHub issue was created and reviewed with the PatternFly community.
  • The pattern design overview.md and design.md templates were used to provide all information, relevant to this design.
  • Any new files have been added to the folder for that pattern.
  • The folder, for brand new patterns, has been titled with the name of the pattern.
  • All images are in an "img" folder and embedded images have the correct filepath.
  • The overview.md, design.md and site.md files, for this design have been commited and pushed to the patternfly-design repository, for review.

beanh66
beanh66 previously approved these changes Jul 23, 2018
Copy link
Member

@beanh66 beanh66 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

@bmignano Thanks for submitting this. I have to admit to not being that familiar with how these bullet charts are used, but I was confused by your description of the "Measure Greater the Max Range" variation that was added. Isn't the blue bar the value and the gray bar the range? Your call outs seem to have these reversed. Not sure. A couple other small things--

  • Should add another jump link at the top of the design page.

  • The mockups on the Overview page should not have callouts as there is nothing for the callouts to reference. This was an oversite with the current documentation, but perhaps you can fix while you are in here.

@bmignano
Copy link
Contributor Author

@mcarrano Great catch! I accidentally reversed the callouts, my mistake. Will fix that and address your other comments once all the reviews are in. Thanks!

@bmignano
Copy link
Contributor Author

Thought this was in pretty good shape overall so I went ahead and made @mcarrano 's changes. @beanh66 @serenamarie125 @mcarrano Please review!

mcarrano
mcarrano previously approved these changes Jul 25, 2018
Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

Looks great @bmignano ! Thanks for making the changes.

beanh66
beanh66 previously approved these changes Jul 26, 2018
Copy link
Member

@beanh66 beanh66 left a comment

Choose a reason for hiding this comment

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

LGTM!

@bmignano bmignano dismissed stale reviews from beanh66 and mcarrano via 2b3ce16 July 26, 2018 13:23
@bmignano
Copy link
Contributor Author

@mcarrano @beanh66 You both already approved, but I just had to make one small change – added a comma after the new jump link on the design page. Just waiting for review from @serenamarie125.

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

OK. I'm still good.

Copy link
Member

@beanh66 beanh66 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

LGTM!

@mcarrano
Copy link
Member

mcarrano commented Aug 3, 2018

Thanks for reviewing @serenamarie125 . I will merge.

@mcarrano mcarrano merged commit 4e89c54 into patternfly:master Aug 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants