Skip to content

Shimmer: fixing import statements#4541

Merged
Vitalius1 merged 3 commits intomicrosoft:masterfrom
Vitalius1:v-vibr/shimmer-importsFix
Apr 13, 2018
Merged

Shimmer: fixing import statements#4541
Vitalius1 merged 3 commits intomicrosoft:masterfrom
Vitalius1:v-vibr/shimmer-importsFix

Conversation

@Vitalius1
Copy link
Copy Markdown
Contributor

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ npm run change

Description of changes

Went through all files in Shimmer component and fixed imports and aligned them for better consistency in using them across the Shimmer component.
Plus added exports to the top level.

@Vitalius1 Vitalius1 requested review from atneik and dzearing April 13, 2018 01:11
@Vitalius1 Vitalius1 changed the title V vibr/shimmer imports fix Shimmer: fixing import statements Apr 13, 2018
Copy link
Copy Markdown
Collaborator

@manishgarg1 manishgarg1 left a comment

Choose a reason for hiding this comment

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

:shipit:

@manishgarg1
Copy link
Copy Markdown
Collaborator

@dzearing, looks good to me. Please give it a look.

@@ -1,5 +1,5 @@
import * as React from 'react';
import { Shimmer } from 'experiments/lib/Shimmer';
import { Shimmer } from '../Shimmer';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this should be @uifabric/experiments/lib/Shimmer

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I might be wrong.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@dzearing This is in the same package and linter is complaining about it. Why not ../Shimmer?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The build is failing if I use @uifabric/experiments/lib/Shimmer

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ERROR in ./src/components/Shimmer/examples/Shimmer.Application.Example.tsx
Module not found: Error: Can't resolve '@uifabric/experiments/lib/Shimmer' in '/Users/vitaliebraga/Desktop/UIFabric/office-ui-fabric-react/packages/experiments/src/components/Shimmer/examples'
 @ ./src/components/Shimmer/examples/Shimmer.Application.Example.tsx 11:16-60
 @ ./src/components/Shimmer/ShimmerPage.tsx
 @ ./src/demo/AppDefinition.tsx
 @ ./src/demo/index.tsx
 @ multi (webpack)-dev-server/client?http://localhost:4322 ./src/demo/index.tsx
ℹ 「wdm」: Failed to compile.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a code example. A user reading it can't use your example when you are using relative paths in examples :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The project webpack config and tsconfig are messed up here and should be resolving @uifabric/experiments.
I'm ok checking this in, but we need to scrub these examples and the configs

@Vitalius1
Copy link
Copy Markdown
Contributor Author

@dzearing Good to merge? The import you commented about seems to be right. See comments above.

@Vitalius1 Vitalius1 merged commit 5a67c36 into microsoft:master Apr 13, 2018
@Vitalius1 Vitalius1 deleted the v-vibr/shimmer-importsFix branch April 16, 2018 01:24
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants