ComponentPage: Add Edit On GitHub buttons to page sections #4424

jordandrako · 2018-03-31T00:25:45Z

Enable them on ActivityItemPage.

Pull request checklist

Include a change request file using $ npm run change

Description of changes

New ComponentPage APIs:

componentUrl Enables the 'View On GitHub' button and programmatically enables the 'Edit' (pencil) buttons. The edit buttons use the URL given, then adds "/docs/${ComponentName}${section}.md" so it points to the relavant markdown file.
editBestPracticesUrl Enables just the edit button for the Best Practices section. Overrides the URL from componentUrl.
editOverviewUrl Enables just the edit button for the Overview section. Overrides the URL from componentUrl.
editDosUrl Enables just the edit button for the Dos section. Overrides the URL from componentUrl.
editDontsUrl Enables just the edit button for the Don'ts section. Overrides the URL from componentUrl.

If only componentUrl is used, the edit buttons are generated from that, the component's name, and the section name. It will not render an edit button if the section doesn't use a React Component, like PageMarkdown, so using HTML like a div won't make an edit button appear in places you can't edit.

All new buttons:

Edit buttons on hover show generated tooltip:

Focus areas to test

Ensure edit buttons are a11y compliant.

Enable them on ActivityItemPage.

jordandrako · 2018-03-31T00:32:44Z

packages/example-app-base/src/components/ComponentPage/ComponentPage.tsx

+      }
+      return (
+        <TooltipHost
+          content={ `Edit ${this.props.componentName} ${readableSection} on GitHub` }


@betrue-final-final I am not sure how accessible this new button is. Is the tooltip with this generated content good enough?

@betrue-final-final Just a friendly bump. I'll have Kirk Myhre take a look at this design-wise too.

@betrue-final-final Are you just worried about the color of the pencil or the addition of the icon and its place in the tab order/narrator in general?

I actually didn't look yet, but now that I am, I do have some.

The "View on Github" appearing as a pivot here is a usability issue actually. It looks like a pivot, but it's actually a link. That will affect everyone.

Is it necessary to be able to edit all those things separately? Do we need that, or just enable editing the whole page?

Let's answer these questions and then evaluate the resulting design for accessibility.

Good comment on the "View on Github" link placement. We'll have to figure out a better place for that within the content of the page.
Each one of those content "chunks" will be editable individually. Based on the way Markdown works and the way we're crafting the community editing experience, we want to provide a quick, easy way for folks to update only the bits that they're responsible for without affecting any others. make sense?

It doesn't have to appear there, or even at all, but I thought it might be convenient.

It is necessary since they are each individual files and have to be that way to reduce non-dev confusion. If we are to implement more than three sections down the road it will be confusing to those not familiar with the component what file to edit if we link to just the /docs folder.

Ok cool. However you might consider "Do's" and "Don'ts" as a single unit. I don't think one person will be assigned to "Do's" and another "Don'ts".

What color are the pencils?

Right, ok that might be tricky on dos and don'ts. Just trying to simplify.

Peter and I thought about how to combine Do's and Don'ts into one file. It'd require some sort of trigger in the markdown file for the markdown-to-jsx component to find and split into the required sections. It'd be too easy to break.

@betrue-final-final The color of the pencils was palette.neutralSecondary but I'm thinking about removing any of the styles and just using IconButton's default styles.

betrue-final-final · 2018-04-09T23:33:13Z

That makes sense. Use components here.

jordandrako · 2018-04-10T19:29:24Z

@kirkmyhre Do you have any suggestions on where to place the "View on GitHub" button, or not have it at all? My plan was for it to link to the component root in the repo ie: https://github.com/OfficeDev/office-ui-fabric-react/tree/master/packages/office-ui-fabric-react/src/components/ActivityItem

jordandrako · 2018-04-10T21:13:46Z

I'll leave this button off this PR, and we can come back to add it in later if we feel it'll be helpful in the future. It sounds like the edit buttons are good to go through, so if I can get approval for those today it'd unblock me converting the rest of the docs over.

jordandrako · 2018-04-10T21:58:07Z

@betrue-final-final Thoughts on approving? Kirk doesn't have permissions to do so.

betrue-final-final · 2018-04-10T22:03:32Z

Let me take a look.

betrue-final-final · 2018-04-11T15:01:43Z

I ran the project, but I'm not seeing the changes. I may not have admin permissions to edit the sections. Maybe I can just take a look over your shoulder.

jordandrako · 2018-04-11T17:42:47Z

This only enables the sections to be enabled if they have markdown, that'll come after this is merged since that conversion relies on the APIs I enabled here.

jordandrako · 2018-04-11T17:43:02Z

Feel free to come by my desk to see how it works.

lynamemi · 2018-04-12T16:05:12Z

packages/example-app-base/src/components/ComponentPage/ComponentPage.scss

+      }
+    }
+
+    .ComponentPage-doSectionHr {


Can you expand 'Hr' into 'Header' or what ever it's for?

It's for the <hr> tag that used to be the :after on the header since it now has to be inside a container for the edit button.

Ah, makes sense now! Maybe another word then? Like 'line' or something? In case it ends up not being an hr tag someday.

Good suggestion

lynamemi · 2018-04-12T16:06:31Z

packages/example-app-base/src/components/ComponentPage/ComponentPage.tsx

+}
+
+export enum ComponentPageSection {
+  Dos = 0,


Should we start these out alphabetized?

lynamemi

Left a couple of nit comments, but looks good, these are exciting changes!

jordandrako · 2018-04-12T19:20:03Z

@mikewheaton @lynamemi Looking the upcoming Fabric site changes, do you think it'd be worthwhile to split this edit button into its own functional component for use elsewhere?

lynamemi · 2018-04-12T19:29:21Z

@jordandrako I like that idea! It definitely has its own thing going on.

Jordan Janzen added 5 commits March 30, 2018 16:20

Add edit buttons and some APIs to ComponentPage.

7382b77

Enable them on ActivityItemPage.

Remove duplicate export

26988d4

Remove test url

e39b916

More documentation, better type safety, and more a11y.

45ca1af

Fix editOverviewUrl prop.

bedff98

jordandrako requested review from Jahnp, betrue-final-final and dzearing March 31, 2018 00:25

npm run changes output

3967779

jordandrako changed the title ~~Edit docs buttons~~ ComponentPage: Add Edit On GitHub buttons API page sections Mar 31, 2018

jordandrako changed the title ~~ComponentPage: Add Edit On GitHub buttons API page sections~~ ComponentPage: Add Edit On GitHub buttons to page sections Mar 31, 2018

jordandrako commented Mar 31, 2018

View reviewed changes

Jordan Janzen and others added 3 commits April 3, 2018 12:07

Make edit buttons go directly to GitHub editor.

484d376

change to let for string replacement

05fb3c4

Merge branch 'master' into edit-docs-buttons

59834df

Jordan Janzen added 3 commits April 9, 2018 17:47

Fix edit url changer

f4712b8

Remove className from edit buttons

7628089

Remove View on Github from Pivot

b109e71

Jordan Janzen added 5 commits April 10, 2018 12:36

Merge branch 'master' into edit-docs-buttons

07d41d6

return undefined if no url before computing urls

beefb6a

Compute URL only if generated

de00933

Add github link method

6a3269e

Add link below overview

5983aed

Remove view button

3a9c52d

Switch from componentName to title

4c25b8e

Jordan Janzen added 5 commits April 10, 2018 18:47

eod

61cf9ef

Add Edit button to Best Practices

03c4ca1

Only render edit button if section has PageMarkdown

29f905d

No best practices

1564123

Remove console log

5f8739c

jordandrako removed the waiting-to-merge label Apr 11, 2018

Jordan Janzen added 4 commits April 11, 2018 10:52

Better switch type safety.

c4fcb7d

Remove spaces from component name.

61afbd7

nit

a5295c0

Fix type error when props are undefined.

9cc735b

jordandrako requested review from lynamemi and mikewheaton April 12, 2018 00:01

removed commented code

05a6870

mikewheaton approved these changes Apr 12, 2018

View reviewed changes

lynamemi reviewed Apr 12, 2018

View reviewed changes

lynamemi approved these changes Apr 12, 2018

View reviewed changes

Jordan Janzen added 3 commits April 12, 2018 10:12

Alphebetize enum

0445d25

Rename class

0de2372

Fix screen reader label

5e55da4

mikewheaton approved these changes Apr 12, 2018

View reviewed changes

Jordan Janzen and others added 2 commits April 12, 2018 18:32

Split button off into subcomponent

525b0ab

fix export

b43c08d

jordandrako merged commit ebbb573 into microsoft:master Apr 13, 2018

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

ComponentPage: Add Edit On GitHub buttons to page sections #4424

ComponentPage: Add Edit On GitHub buttons to page sections #4424

Uh oh!

Conversation

jordandrako commented Mar 31, 2018 • edited 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

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!

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!

betrue-final-final commented Apr 9, 2018

Uh oh!

jordandrako commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jordandrako commented Apr 10, 2018

Uh oh!

jordandrako commented Apr 10, 2018

Uh oh!

betrue-final-final commented Apr 10, 2018

Uh oh!

betrue-final-final commented Apr 11, 2018

Uh oh!

jordandrako commented Apr 11, 2018

Uh oh!

jordandrako commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

lynamemi left a comment

Choose a reason for hiding this comment

Uh oh!

jordandrako commented Apr 12, 2018

Uh oh!

lynamemi commented Apr 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jordandrako commented Mar 31, 2018 •

edited

Loading

jordandrako commented Apr 10, 2018 •

edited

Loading

jordandrako commented Apr 11, 2018 •

edited

Loading