-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Charting #4954
Charting #4954
Conversation
Would you have a chance to give us 30 min demo of the work you've done here? |
Can you merge master and rerun shrinkwrap? |
…c-react into charting # Conflicts: # common/changes/@uifabric/example-app-base/applied-prettier_2018-06-02-22-17.json # common/changes/@uifabric/experiments/applied-prettier_2018-06-02-22-17.json # common/changes/@uifabric/fabric-website/applied-prettier_2018-06-02-22-17.json # common/changes/@uifabric/file-type-icons/applied-prettier_2018-06-02-22-17.json # common/changes/@uifabric/icons/applied-prettier_2018-06-02-22-17.json # common/changes/@uifabric/jest-serializer-merge-styles/applied-prettier_2018-06-02-22-17.json # common/changes/@uifabric/merge-styles/applied-prettier_2018-06-02-22-17.json # common/changes/@uifabric/styling/applied-prettier_2018-06-02-22-17.json # common/changes/@uifabric/utilities/applied-prettier_2018-06-02-22-17.json # common/changes/@uifabric/variants/applied-prettier_2018-06-02-22-17.json # common/changes/office-ui-fabric-react/applied-prettier_2018-06-02-22-17.json # common/changes/office-ui-fabric-react/commandbar-fabricjs_2018-06-04-19-00.json
@@ -0,0 +1,11 @@ | |||
{ | |||
"changes": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should not be showing up as a new file created by you..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pulled upstream/master into my local branch, committed, and then pushed to origin. I'd definitely appreciate tips for how to avoid having these files show up in my PRs.
import { ITheme, IStyle } from '../../Styling'; | ||
import { IStyleFunctionOrObject } from '../../Utilities'; | ||
|
||
export interface IVerticalBarChart { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IVerticalBarChart [](start = 17, length = 17)
Empty interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see this is sort of a pattern we have at least in some places atm, although it is a little weird to see it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It provides a place to add a public interface. I am ok with it, moreso than people not realizing there should be one.
theme: ITheme; | ||
className?: string; | ||
width: number; | ||
height: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should jsdoc all these (I looked at other *StyleProps and they were documented)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be done in a future pr, but agree.
<p>Within an npm project, you should install the package and save it as a dependency:</p> | ||
|
||
<div className="ms-GettingStartedPage-code"> | ||
<Highlight className="bash">npm install --save office-ui-fabric-react</Highlight> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
office-ui-fabric-react [](start = 57, length = 22)
A lot of the things on this page is not in sync with the package/page title..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same as the Getting Started page from the experiments package, which also doesn't seem to match up with what's in experiments. I think updating these pages to match their packages is a separate work item.
There is something wrong with the way master was merged into this branch and as such there are a lot of files that incorrectly show up as your changes.. :\ |
|
||
import { VerticalBarChartBasicExample } from './examples/VerticalBarChart.Basic.Example'; | ||
|
||
const VerticalBarChartBasicExampleCode = require('!raw-loader!@uifabric/charting/src/components/VerticalBarChart/examples/VerticalBarChart.Basic.Example.tsx') as string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may conflict with @kenotron pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happens to be okay because it is not OUFR package itself. So it will need to be refactored before being promoted to OUFR.
* master: Part 2 of demo page refactor (microsoft#5089) Update jest.js fixing official example page and datepicker/calendar components using… (microsoft#5108) Don't run prettier and tslint in parallel as it might cause conflicts (microsoft#5100) FocusTrapZone - restore last focused descendant element (microsoft#5103) Coachmark/TeachingBubble: Fix SCSS selectors for buttons and Close Icon (microsoft#4835) HoverCard: IE11 fix (microsoft#5105)
Cleanup my mess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is mostly an experimental package, I think it's okay to get in a rough cut.
|
||
import { VerticalBarChartBasicExample } from './examples/VerticalBarChart.Basic.Example'; | ||
|
||
const VerticalBarChartBasicExampleCode = require('!raw-loader!@uifabric/charting/src/components/VerticalBarChart/examples/VerticalBarChart.Basic.Example.tsx') as string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happens to be okay because it is not OUFR package itself. So it will need to be refactored before being promoted to OUFR.
@@ -0,0 +1 @@ | |||
export * from 'office-ui-fabric-react/lib/Styling'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the Utilities.ts don't seem to be needed - perhaps we can just remove these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're used in VerticalBarChart.types.ts
* master: Charting (microsoft#4954) Deprecation lint rule! (microsoft#5109) Implement selection for selected items list (microsoft#5036) Ignore common/changes and don't prettify json files (microsoft#5112) Part 2 of demo page refactor (microsoft#5089) Update jest.js fixing official example page and datepicker/calendar components using… (microsoft#5108) Don't run prettier and tslint in parallel as it might cause conflicts (microsoft#5100) FocusTrapZone - restore last focused descendant element (microsoft#5103) Coachmark/TeachingBubble: Fix SCSS selectors for buttons and Close Icon (microsoft#4835) HoverCard: IE11 fix (microsoft#5105)
* master: Charting (microsoft#4954) Deprecation lint rule! (microsoft#5109) Implement selection for selected items list (microsoft#5036) Ignore common/changes and don't prettify json files (microsoft#5112)
* master: (31 commits) Charting (microsoft#4954) Deprecation lint rule! (microsoft#5109) Implement selection for selected items list (microsoft#5036) Ignore common/changes and don't prettify json files (microsoft#5112) Part 2 of demo page refactor (microsoft#5089) Update jest.js fixing official example page and datepicker/calendar components using… (microsoft#5108) Don't run prettier and tslint in parallel as it might cause conflicts (microsoft#5100) FocusTrapZone - restore last focused descendant element (microsoft#5103) Coachmark/TeachingBubble: Fix SCSS selectors for buttons and Close Icon (microsoft#4835) HoverCard: IE11 fix (microsoft#5105) FocusTrapZone bug allows breaking out of the trap (microsoft#4898) Applying package updates. Update ISSUE_TEMPLATE.md Experiment/Nav component: hide nav group header if all the links under it are hidden (microsoft#5095) Add optional prop to not dismiss Callout on focus loss (microsoft#5092) Experiments: moves ShimmerTile from Shimmer to Tile. (microsoft#5090) Run jest in parallel on Windows (microsoft#5096) Applying package updates. Major bump jest-serializer-merge-styles ...
Pull request checklist
$ npm run change
Description of changes
New experimental charting package with VerticalBarChart component.
Focus areas to test
(optional)
Microsoft Reviewers: Open in CodeFlow