Demo Page Refactor Part 1 by kenotron · Pull Request #5055 · microsoft/fluentui

kenotron · 2018-06-01T03:27:47Z

Pull request checklist

Addresses an existing issue: Fixes #0000
Include a change request file using $ npm run change
Microsoft Alias (if you have one): kchau

Description of changes

This is Part 1 / 3 of a demo page refactor to get rid of circular deps in our fabric / demo / example-app-base. In this part, I'm simply trying to establish the pattern with ONE converted page to keep this checkin tiny and easy to review.

Part 2 will convert all other pages to use this format under fabric
Part 3 will actually remove the fabric -> example-app-base dependency

Focus areas to test

(optional)

Microsoft Reviewers: Open in CodeFlow

…ement IS stateless component

cliffkoh · 2018-06-01T19:20:51Z

packages/office-ui-fabric-react/src/components/ActivityItem/ActivityItemPage.tsx

 import { ActivityItemPersonaExample } from './examples/ActivityItem.Persona.Example';
 import { ActivityItemCompactExample } from './examples/ActivityItem.Compact.Example';
+import { IDemoPageProps } from '../../demo/components/IDemoPageProps';
+import { DemoPage } from 'office-ui-fabric-react/lib/demo/components/DemoPage';


office-ui-fabric-react/lib/demo [](start = 26, length = 31)

Did you mean to do a relative path import from here?

Yes, I did actually mean to do relative.

cliffkoh · 2018-06-01T19:22:05Z

packages/office-ui-fabric-react/src/demo/components/DemoPage.tsx

+import { ComponentPage, ExampleCard, PropertiesTableSet, PageMarkdown } from '@uifabric/example-app-base';
+import * as React from 'react';
+import { ComponentStatus } from 'office-ui-fabric-react/lib/demo/ComponentStatus/ComponentStatus';
+


auto-import keeps giving me non relative stuff

cliffkoh · 2018-06-01T19:24:12Z

packages/office-ui-fabric-react/src/demo/components/IDemoPageProps.ts

+  donts: string;
+  bestPractices: string;
+  isHeaderVisible: boolean;
+}


2 things:

we no longer call props pages ComponentProps.ts in Fabric. Instead, it is presently .types.ts

need js doc here :)

does it matter that this isn't going to be something we document with api documenter tool?

cliffkoh · 2018-06-01T19:26:32Z

It would be helpful for code reviewers if you illustrated in your PR description the before/after so it is clearer how the circular dependency was before, so that reviewers can verify that the circular dependency is broken up now...

kenotron · 2018-06-01T21:30:28Z

I won't be able to break up the circular dependency until I'm done with part 3, so I can speak to just one file and its implications, @cliffkoh .

Currently, we have a circular dependency of OUFR + Demo -> example-app-base -> OUFR

What I seek to do is to move Demo pages out of OUFR (retaining the important examples, overview, etc in an exported JSON like I have here inside ActivityItemPage.tsx). So it'll look like this:

Demo (consumes .md & page props inside OUFR via relative or webpack aliasing) -> example-app-base -> OUFR

cliffkoh · 2018-06-01T22:57:29Z

packages/office-ui-fabric-react/src/demo/components/DemoPage.types.ts

@@ -0,0 +1,45 @@
+import { IComponentStatusProps } from 'office-ui-fabric-react/lib/demo/ComponentStatus/ComponentStatus.types';


Should be a relative path import

cliffkoh · 2018-06-01T23:00:24Z

packages/office-ui-fabric-react/src/demo/components/DemoPage.tsx

+import * as React from 'react';
+import { ComponentStatus } from '../ComponentStatus/ComponentStatus';
+
+export const DemoPage = (componentPageProps: IDemoPageProps) => {


Please kindly type as React.StatelessComponent :) The current declaration isn't equivalent from a typing POV.

cliffkoh · 2018-06-01T23:03:07Z

packages/office-ui-fabric-react/src/demo/components/DemoPage.types.ts

+  /** title that goes into the header */
+  title: string;
+
+  /** name of the component being documented */


I know all these are not making it today, to the documentation page, but ultimately contributors to the OUFR library will be interacting with it when they fill out the IDemoPageProps object in each of the example page they craft.

So we really should have the same quality bar in our comments as we expect the directly public-facing documentation that makes it to the website.

You know.. broken window theory aside, I believe the core team needs to set a good example.

So Name of the component being documented. instead of name of the component being documented.

I think we might want to write down the bar for documentation. I hadn't found a wiki page / readme on it so far. Perhaps I'll add it once I get this checked in.

cliffkoh

Thanks for the comment describing your overall approach. 👍

* master: (274 commits) Demo Page Refactor Part 1 (microsoft#5055) SplitButton: Add aria-roledescription (microsoft#5062) Add addins sketch toolkit link (microsoft#5052) Dropdown title (microsoft#4983) Allow for more control over event handling for keytips (microsoft#5064) Build time speed improvements (microsoft#4965) Coachmark: Positioning fixes (microsoft#4995) Applying package updates. Experiments/Nav component: display "show more" link only if there is atleast one hidden link (microsoft#5057) Add pointerup listener to exit keytip mode (microsoft#5051) Update PULL_REQUEST_TEMPLATE.md Update ISSUE_TEMPLATE.md Shimmer: resolve conflicts (microsoft#5034) Invalid ARIA attributes: Fix empty values (microsoft#5040) ComboBox: Correct invalid ARIA attributes. (microsoft#4873) (microsoft#5001) ComboBox: Fix submit pending value (microsoft#5048) FocusTrapZone - restore last focused descendant element (microsoft#4897) Applying package updates. Take 2 of the require.resolve change. This time using the "resolve" pkg (microsoft#5031) fixing webpack config to allow rush build to complete on a small VM (microsoft#5037) ...

* master: (111 commits) Moving a variable to be defined sooner so that it is there when failures occur. Fix icon imports (microsoft#5069) MessageBar: More visible HC color for dismiss and expand buttons (microsoft#5061) Fix DetailsList accessibility and add more ARIA hooks (microsoft#5066) Update jest (microsoft#5068) Demo Page Refactor Part 1 (microsoft#5055) SplitButton: Add aria-roledescription (microsoft#5062) Add addins sketch toolkit link (microsoft#5052) Dropdown title (microsoft#4983) Allow for more control over event handling for keytips (microsoft#5064) Build time speed improvements (microsoft#4965) Coachmark: Positioning fixes (microsoft#4995) Applying package updates. Experiments/Nav component: display "show more" link only if there is atleast one hidden link (microsoft#5057) Add pointerup listener to exit keytip mode (microsoft#5051) Update PULL_REQUEST_TEMPLATE.md Update ISSUE_TEMPLATE.md Shimmer: resolve conflicts (microsoft#5034) Invalid ARIA attributes: Fix empty values (microsoft#5040) ComboBox: Correct invalid ARIA attributes. (microsoft#4873) (microsoft#5001) ...

kenotron added 2 commits May 31, 2018 19:44

convert componentPage to a set of pageProps

5d9b0ef

rename to DemoPage component because a function that returns React El…

9af2afd

…ement IS stateless component

kenotron requested a review from dzearing June 1, 2018 03:27

cliffkoh reviewed Jun 1, 2018

View reviewed changes

kenotron added 3 commits June 1, 2018 14:31

renamed file to fit convention of repo; fixed to use relative imports

f08f3fc

adding jsdoc

88148c3

demo page turn from function to lambda

b6d500d

cliffkoh reviewed Jun 1, 2018

View reviewed changes

cliffkoh approved these changes Jun 1, 2018

View reviewed changes

kenotron added 2 commits June 1, 2018 22:12

fixed comments and relative paths some more

b4bc564

add change file

a80e9f3

kenotron merged commit 1506d4c into microsoft:master Jun 2, 2018

kenotron deleted the demo branch June 2, 2018 13:15

microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019

		@@ -0,0 +1,45 @@
		import { IComponentStatusProps } from 'office-ui-fabric-react/lib/demo/ComponentStatus/ComponentStatus.types';

Conversation

kenotron commented Jun 1, 2018 • edited by fabricteam Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist

Description of changes

Focus areas to test

Microsoft Reviewers: Open in CodeFlow

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cliffkoh commented Jun 1, 2018

Uh oh!

kenotron commented Jun 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cliffkoh Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cliffkoh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kenotron commented Jun 1, 2018 •

edited by fabricteam

Loading

cliffkoh Jun 1, 2018 •

edited

Loading

cliffkoh left a comment •

edited

Loading