feat(report): support ability to export to CodePen #2252

sean-beard · 2020-02-28T21:36:07Z

Description of changes

These changes add the option to export reports to CodePen in addition to an HTML file. There is a relevant open issue detailing a need for this type of functionality.

I wasn't quite sure what the design should look like for this so I've went ahead and used a split PrimaryButton from office-ui-fabric-react in the ExportDialog component but I'm definitely open to alternative approaches!

I was also considering splitting out the HTML and CSS before sending the payload to CodePen but I figured I'd get some feedback on the changes within this PR before moving forward with more in-depth functionality.

Pull request checklist

Addresses an existing issue: export: provide hosted export targets #1739
Ran yarn fastpass
Added/updated relevant unit test(s) (and ran yarn test)
Verified code coverage for the changes made. Check coverage report at: <rootDir>/test-results/unit/coverage
PR title AND final merge commit title both start with a semantic tag (fix:, chore:, feat(feature-name):, refactor:). Check workflow guide at: <rootDir>/docs/workflow.md
(UI changes only) Added screenshots/GIFs to description above
(UI changes only) Verified usability with NVDA/JAWS

msftclas · 2020-02-28T21:36:20Z

All CLA requirements met.

codecov-io · 2020-02-28T21:47:12Z

Codecov Report

Merging #2252 into master will decrease coverage by 0.07%.
The diff coverage is 52.00%.

@@            Coverage Diff             @@
##           master    #2252      +/-   ##
==========================================
- Coverage   95.75%   95.68%   -0.08%     
==========================================
  Files         962      963       +1     
  Lines       14944    14968      +24     
  Branches      999     1002       +3     
==========================================
+ Hits        14310    14322      +12     
- Misses        513      523      +10     
- Partials      121      123       +2

Impacted Files	Coverage Δ
...-export/services/codepen-report-export-service.tsx	`33.33% <33.33%> (ø)`
src/DetailsView/components/export-dialog.tsx	`86.66% <69.23%> (-13.34%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02291cf...6d1298f. Read the comment docs.

smoralesd

Hi @sean-beard, thank you so much for deciding to contribute to Accessibility Insights for Web, this is one of the first feature request contributions we have and the team is super excited to see this through.

We'd like to tackle this from 2 different angles: UI and code. I have started internal conversations to help move this forward on both fronts.

On one hand, yours it's a very good first approach that will allow us to have a short back-and-forth loop about how the UI should look in the finalized feature.

On the other hand, I channeled the team comments around the code. We always try to keep a high bar and we expect addressing this comments will allows us (you and the team) to keep (or even improve) the level of quality.

If you have any questions, please don't hesitate reaching out.

Sebastián.

smoralesd · 2020-03-02T18:02:56Z

src/DetailsView/components/export-dialog.tsx

-                </PrimaryButton>
+                <PrimaryButton
+                    text="Export"
+                    primary


The primary option is not needed. The <PrimaryButton> already handles that.

smoralesd · 2020-03-02T18:08:28Z

src/DetailsView/components/report-export-component.tsx

@@ -67,6 +67,45 @@ export class ReportExportComponent extends React.Component<
        this.setState({ exportDescription, exportName, isOpen: true });
    };

+    private exportToCodePen = () => {


This is a great opportunity to introduce a Strategy Pattern on how we export the report.
One strategy will be "export html report to file (in the local system)"; the second strategy will be "export html report to code pen".
Handling the different approaches with a Strategy Pattern allows to make <ReportExportComponent> open/close, i.e. we enable ourselves to add more export strategies without the need to modify this component.
We probably need a container so we can use the ExportFormat enum to get the right strategy to apply when the user selects the export button or any of the options available on the contextual menu of the button.

smoralesd · 2020-03-02T18:16:12Z

src/DetailsView/components/report-export-component.tsx

+        const { htmlGenerator } = this.props;
+        const { exportName, exportDescription } = this.state;
+
+        const form = document.createElement('form');


We avoid using global dependencies as much as we can. Using document in here makes this component to "be aware" it's running in a browser, and even though that's a fair assumption, it's also an easy one to avoid. Not assuming the code is running on a full-fledged browser allow for at least to things: 1) running the code not in a browser (like in a test run scenario without the need of any polyfill); 2) make it easier to unit test, as you don't need to rely on the test framework or any other library to provide for the document object; in this scenario, having a mock of whatever class we end up using will suffice.

That being said, if we decide to move forward with the Strategy pattern mention on one of my other comments, this code should be move to the ExportToCodePenStrategy class. We'll still need to not directly use document but ExportToCodePenStrategy can receive a FormFactory if you will.

smoralesd · 2020-03-02T18:18:51Z

src/DetailsView/components/report-export-component.tsx

+    };
+
+    private onExportClick = (format: ExportFormat) => {
+        switch (format) {


If we use the Strategy Pattern, then onExportClick does not need to have the switch statement here. As long as we have a mean to retrieve the appropriate strategy base on format: ExportFormat we should be good.

smoralesd · 2020-03-11T17:59:14Z

Hey @sean-beard I noticed you pushed some new changes. Should I go ahead and review them?

sean-beard · 2020-03-13T16:17:57Z

Hey @smoralesd, sure, feel free to review!

smoralesd · 2020-03-16T23:39:42Z

/azp run

azure-pipelines · 2020-03-16T23:39:57Z

Azure Pipelines successfully started running 1 pipeline(s).

smoralesd

This seems like very good progress.
I have a couple of concerns (in general):

Global dependencies (like how we are using ReportExportServiceProviderImpl in the ExportDialog (left a comment there)
React hooks introduction: this is something we have talked about in the team but this will be the first time someone actually use hooks. Some of the main concerns are (again) global dependencies and how to properly unit test this. Left a comment on a file with more details.

smoralesd · 2020-03-17T21:09:53Z

src/DetailsView/components/export-dialog.tsx

+                        items: ReportExportServiceProviderImpl.all().map(service => ({
+                            key: service.key,
+                            text: service.displayName,
+                            onClick: (e: React.MouseEvent<HTMLButtonElement>) => {


This onClick function does not seem to be covered by unit test.

smoralesd · 2020-03-17T21:10:12Z

src/DetailsView/components/export-dialog.tsx

+                        fileName={props.fileName}
+                        description={props.description}
+                        html={props.html}
+                        onSubmit={() => {


The onSubmit function here does not seem to be covered by unit test

smoralesd · 2020-03-17T21:13:16Z

src/DetailsView/components/export-dialog.tsx

@@ -27,18 +29,24 @@ export interface ExportDialogDeps {
 }

 export const ExportDialog = NamedFC<ExportDialogProps>('ExportDialog', props => {
+    const [format, setFormat] = React.useState<ExportFormat | null>(null);


@waabid @dbjorge This is the first time we'll be introducing a react hook.
I'm not against this but I do have concerns, specially the fact that this is a global dependency and how to properly test the state changes (I'm guessing the testing part is a solved problem at this point but I may be wrong).
Any thoughts?

I don't have a particularly strong opinion on hooks vs class components as concepts, but I think it's better for maintainability to try to be consistent within a given codebase; I would lean towards using a class component instead for consistency's sake.

smoralesd · 2020-03-17T21:24:48Z

src/report-export/services/codepen-report-export-service.tsx

+
+const CodePenReportExportServiceKey: ExportFormat = 'codepen';
+
+const exportForm = NamedFC<ExportFormProps>(


The exportForm seems interesting enough that we should have unit tests for it.
The use of react hooks calls my attention here: currently on the repo, we don't use hooks so we don't have any example on how to unit tests this. It would be a very good contribution to write the first unit test around react hooks! If that seems to convoluted, there is always the possibility to go back to a more "traditional" react component so we can follow other examples in this repo on how to test it.

smoralesd · 2020-03-17T21:28:54Z

src/DetailsView/components/export-dialog.tsx

@@ -48,6 +56,8 @@ export const ExportDialog = NamedFC<ExportDialogProps>('ExportDialog', props =>
    };

    const fileURL = props.deps.fileURLProvider.provideURL([props.html], 'text/html');
+    const exportService = ReportExportServiceProviderImpl.forKey(format);


ReportExportServiceProviderImpl is being use as a global dependency. Among other unwanted side effects, this forces the unit test to not be properly isolated (by depending on a concrete class/instance/implementation that's not the unit being tested).
One way to solve this is to follow the "deps pattern" we have across the repo:
Add a new property under ExportDialogDeps where we can pass an instance of the ReportExportServiceProvider. For the extension itself, this instance will be ReportExportServiceProviderImpl but for the unit test it can be a mock which does not need to change every time ReportExportServicePRoviderImpl changes, making the unit test properly isolated of other implementation details.

smoralesd · 2020-03-23T16:22:48Z

/azp run

azure-pipelines · 2020-03-23T16:23:03Z

Azure Pipelines successfully started running 1 pipeline(s).

(cherry picked from commit 75a06f6)

(cherry picked from commit 382b7ae)

(cherry picked from commit a87c488)

dbjorge

Thanks again for the contribution, Sean; apologies for the delays in getting it merged! You can expect to see it as a "Preview Feature" in the next release :)

Taking over from Sebastian

sean-beard requested a review from a team as a code owner February 28, 2020 21:36

smoralesd suggested changes Mar 2, 2020

View reviewed changes

sean-beard force-pushed the sb/eval-export-codepen branch 2 times, most recently from 030a46b to 840c067 Compare March 6, 2020 23:28

sean-beard force-pushed the sb/eval-export-codepen branch 2 times, most recently from c2e964d to 11a4947 Compare March 16, 2020 01:42

smoralesd previously requested changes Mar 17, 2020

View reviewed changes

sean-beard force-pushed the sb/eval-export-codepen branch from 11a4947 to 6d1298f Compare March 21, 2020 03:55

sean-beard force-pushed the sb/eval-export-codepen branch from 6d1298f to c6e47fb Compare March 28, 2020 18:42

sean-beard and others added 12 commits March 31, 2020 17:10

feat(report): support ability to export to CodePen

4e2767b

follow deps pattern for ReportExportServiceProvider

454a7ae

convert codepen export form to class component

1b9825a

add test for exporting to codepen

17dee94

add feature flag for the new export report feature

a346f01

(cherry picked from commit 75a06f6)

use FlaggedComponent on the export button

b69694f

(cherry picked from commit 382b7ae)

update feature flag title and description

e93b365

(cherry picked from commit a87c488)

fix test after cherry pick

e20199d

alphabetize imports

2f2791b

run all export dialog tests

8c3dc23

add codepen export form snapshot test

2860a4c

fastpass fix

eba86b2

sean-beard force-pushed the sb/eval-export-codepen branch from c6e47fb to eba86b2 Compare March 31, 2020 23:12

smoralesd and others added 11 commits April 6, 2020 13:28

Merge branch 'master' into sb/eval-export-codepen

5b167a7

run yarn format:fix

0eb4f70

convert the feature flag from public to private

f6c5530

update export report snapshot

891094b

add unit test for codepen report export service export form

1fad029

alphabetize imports

4169db7

Use public rather than private flag

09af187

Merge branch 'master' into sb/eval-export-codepen

594f072

Fix logical merge conflict with unified export feature

9351799

Update snapshots for CommandBar unit tests

0b3e043

Update preview features e2e test to account for new public flag

9dfbf4d

dbjorge approved these changes Apr 14, 2020

View reviewed changes

dbjorge merged commit 1c8ff9b into microsoft:master Apr 14, 2020

juchampa mentioned this pull request Jun 27, 2020

export: provide hosted export targets #1739

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(report): support ability to export to CodePen #2252

feat(report): support ability to export to CodePen #2252

sean-beard commented Feb 28, 2020

msftclas commented Feb 28, 2020 •

edited

Loading

codecov-io commented Feb 28, 2020 •

edited

Loading

smoralesd left a comment

smoralesd Mar 2, 2020

smoralesd Mar 2, 2020

smoralesd Mar 2, 2020

smoralesd Mar 2, 2020

smoralesd commented Mar 11, 2020

sean-beard commented Mar 13, 2020 •

edited

Loading

smoralesd commented Mar 16, 2020

azure-pipelines bot commented Mar 16, 2020

smoralesd left a comment

smoralesd Mar 17, 2020

smoralesd Mar 17, 2020

smoralesd Mar 17, 2020

dbjorge Mar 18, 2020

smoralesd Mar 17, 2020

smoralesd Mar 17, 2020 •

edited

Loading

smoralesd commented Mar 23, 2020

azure-pipelines bot commented Mar 23, 2020

dbjorge left a comment


		const CodePenReportExportServiceKey: ExportFormat = 'codepen';

		const exportForm = NamedFC<ExportFormProps>(

feat(report): support ability to export to CodePen #2252

feat(report): support ability to export to CodePen #2252

Conversation

sean-beard commented Feb 28, 2020

Description of changes

Pull request checklist

msftclas commented Feb 28, 2020 • edited Loading

codecov-io commented Feb 28, 2020 • edited Loading

Codecov Report

smoralesd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoralesd commented Mar 11, 2020

sean-beard commented Mar 13, 2020 • edited Loading

smoralesd commented Mar 16, 2020

azure-pipelines bot commented Mar 16, 2020

smoralesd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoralesd Mar 17, 2020 • edited Loading

Choose a reason for hiding this comment

smoralesd commented Mar 23, 2020

azure-pipelines bot commented Mar 23, 2020

dbjorge left a comment

Choose a reason for hiding this comment

msftclas commented Feb 28, 2020 •

edited

Loading

codecov-io commented Feb 28, 2020 •

edited

Loading

sean-beard commented Mar 13, 2020 •

edited

Loading

smoralesd Mar 17, 2020 •

edited

Loading