-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Experimental Chiclet Component #4678
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
Changes from 35 commits
fb26b91
ad94997
542fcf8
c6f0aab
5d7f668
56f52c9
15ccf73
1012e4c
ee82f29
fd42f49
fc90f50
3fa9d9e
3df9860
a2b0fa5
2e5978d
e497323
d0b31a2
d3a9f0d
acbcbc2
26cb7d0
bbacc49
600e4c8
7742c47
2f3ca5b
4e44852
64af2e7
5b4d21d
4e87878
b9581b7
3719108
7d1104d
f873b97
5020b32
eb3b959
a44db3b
4ba3924
65fba98
2b65727
3965f9a
c91dfe3
806aff1
cbe9c0f
eba2185
00c9ca3
6d762d2
7a35619
db18be7
02849e3
ada5ffe
9a1b9c6
4756ed0
a0d6e73
68b9102
1abe186
d353ed7
6bad6f0
49c756b
972d37d
16156a8
275bbc2
db80bf2
70badd3
5b381fb
8f17013
9a7d9e8
be9118f
b33c5b8
51f1ba8
679e0a6
c404200
7a550dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,4 +30,4 @@ | |
| "devDependencies": { | ||
| "@microsoft/rush": "4.3.0" | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export * from './components/Chiclet/index'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| const baseProductionCdnUrl = 'https://az742526.vo.msecnd.net/files/odsp-next-release-odc_2018-04-13_20180418.001/odsp-media/images/apps/'; | ||
|
|
||
| export const ChicletTestImages = { | ||
| iconWordDoc: baseProductionCdnUrl + 'word_16x1.svg', | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| import * as React from 'react'; | ||
| import { | ||
| BaseComponent, | ||
| customizable, | ||
| classNamesFunction | ||
| } from '../../Utilities'; | ||
| import { Chiclet } from './Chiclet'; | ||
| import { ChicletSize } from './Chiclet.types'; | ||
| import { extractMetaTags } from './OpenGraph'; | ||
| import { IBaseChicletProps, IBaseChicletStyles, IBaseChicletStyleProps } from './BaseChiclet.types'; | ||
|
|
||
| const getClassNames = classNamesFunction<IBaseChicletStyleProps, IBaseChicletStyles>(); | ||
|
|
||
| @customizable('BaseChicletBase', ['theme']) | ||
| export class BaseChicletBase extends BaseComponent<IBaseChicletProps, any> { | ||
| private _classNames: { [key in keyof IBaseChicletStyles]: string }; | ||
|
|
||
| constructor(props: IBaseChicletProps) { | ||
| super(props); | ||
|
|
||
| let chicletCardProps = extractMetaTags(this.props.url); | ||
| this.state = { chicletCardProps: chicletCardProps }; | ||
| } | ||
|
|
||
| public render() { | ||
| const { size, footer, getStyles, theme } = this.props; | ||
| const { chicletCardProps } = this.state; | ||
|
|
||
| this._classNames = getClassNames(getStyles, { theme: theme! }); | ||
|
|
||
| return ( | ||
| <Chiclet chicletCardProps={ chicletCardProps } size={ size ? size : ChicletSize.medium } footer={ footer } /> | ||
| ); | ||
| } | ||
|
|
||
| public componentWillReceiveProps(nextProps: any) { | ||
| if (this.props.url != nextProps.url) { | ||
| this.setState({ chicletCardProps: extractMetaTags(this.props.url) }); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| import { IBaseChicletStyleProps, IBaseChicletStyles } from './BaseChiclet.types'; | ||
|
|
||
| export const getStyles = ( | ||
| props: IBaseChicletStyleProps | ||
| ): IBaseChicletStyles => { | ||
| const { theme } = props; | ||
|
|
||
| return ({ | ||
| root: {} | ||
| }); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| import { styled } from '../../Utilities'; | ||
| import { | ||
| IBaseChicletProps, | ||
| IBaseChicletStyleProps, | ||
| IBaseChicletStyles | ||
| } from './BaseChiclet.types'; | ||
| import { getStyles } from './BaseChiclet.styles'; | ||
| import { BaseChicletBase } from './BaseChiclet.base'; | ||
|
|
||
| export const BaseChiclet = styled<IBaseChicletProps, IBaseChicletStyleProps, IBaseChicletStyles>( | ||
| BaseChicletBase, | ||
| getStyles | ||
| ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| import * as React from 'react'; | ||
| import { BaseChicletBase } from './BaseChiclet.base'; | ||
| import { ChicletSize } from './Chiclet.types'; | ||
| import { IStyleFunction } from '../../Utilities'; | ||
| import { | ||
| IStyle, | ||
| ITheme | ||
| } from '../../Styling'; | ||
|
|
||
| export interface IBaseChiclet { | ||
|
|
||
| } | ||
|
|
||
| export interface IBaseChicletProps extends React.Props<BaseChicletBase> { | ||
| /** | ||
| * Optional callback to access the IBaseChiclet interface. Use this instead of ref for accessing | ||
| * the public methods and properties of the component. | ||
| */ | ||
| componentRef?: (component: IBaseChiclet | null) => void; | ||
|
|
||
| /** @todo: description */ | ||
| getStyles?: IStyleFunction<IBaseChicletStyleProps, IBaseChicletStyles>; | ||
|
|
||
| /** | ||
| * Optional class for chiclet. | ||
| */ | ||
| className?: string; | ||
|
|
||
| /** | ||
| * Sharing link | ||
| */ | ||
| url: string; | ||
|
|
||
| /** | ||
| * Chiclet size to render | ||
| */ | ||
| size?: ChicletSize; | ||
|
|
||
| /** | ||
| * Footer to render for the component. | ||
| */ | ||
| footer?: React.ReactElement<any> | ||
|
|
||
| /** | ||
| * Theme for the component. | ||
| */ | ||
| theme?: ITheme; | ||
| } | ||
|
|
||
| export interface IBaseChicletStyleProps { | ||
| /** | ||
| * Theme for the component. | ||
| */ | ||
| theme?: ITheme; | ||
| } | ||
|
|
||
| export interface IBaseChicletStyles { | ||
| root?: IStyle; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import * as React from 'react'; | ||
| import { IChicletProps, ChicletSize } from './Chiclet.types'; | ||
| import { IChicletCardProps } from './ChicletCard.types'; | ||
| import { ChicletCard } from './ChicletCard'; | ||
|
|
||
| export class ChicletBase extends React.Component<IChicletProps, IChicletCardProps> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the difference between this and BaseChicletBase?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my response to Pete's comment above. :) |
||
| public render() { | ||
| const { chicletCardProps, size, footer } = this.props; | ||
|
|
||
| switch (size) { | ||
| case ChicletSize.medium: | ||
| return ( | ||
| <ChicletCard { ...chicletCardProps } onClick={ this._onClick } footer={ footer } /> | ||
| ); | ||
| // @todo: handle other types of chiclets | ||
| default: | ||
| return ( | ||
| <ChicletCard { ...chicletCardProps } onClick={ this._onClick } footer={ footer } /> | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| private _onClick(): void { // @todo: default click handler | ||
| console.log("You clicked the Chiclet"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is this intended to be included in the official release?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll need to check with the Design team to see what the expected behavior of a Chiclet onClick should be, so this is just a placeholder for now! I'll make sure to change it before release. :) |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| import { IChicletStyleProps, IChicletStyles } from './Chiclet.types'; | ||
|
|
||
| export const getStyles = ( | ||
| props: IChicletStyleProps | ||
| ): IChicletStyles => { | ||
| const { theme } = props; | ||
|
|
||
| return ({ | ||
| root: {} | ||
| }); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| import { styled } from '../../Utilities'; | ||
| import { | ||
| IChicletProps, | ||
| IChicletStyleProps, | ||
| IChicletStyles | ||
| } from './Chiclet.types'; | ||
| import { getStyles } from './Chiclet.styles'; | ||
| import { ChicletBase } from './Chiclet.base'; | ||
|
|
||
| export const Chiclet = styled<IChicletProps, IChicletStyleProps, IChicletStyles>( | ||
| ChicletBase, | ||
| getStyles | ||
| ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| import * as React from 'react'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's split this file into 3 one for each component: ChicletCard.types.ts, Chiclet.types.ts and BaseChiclet.types.ts |
||
| import { Chiclet } from './Chiclet'; | ||
| import { | ||
| IChicletCardProps, | ||
| IChicletCardStyleProps, | ||
| IChicletCardStyles | ||
| } from './ChicletCard.types'; | ||
| import { | ||
| IStyle, | ||
| ITheme | ||
| } from '../../Styling'; | ||
| import { IStyleFunction } from '../../Utilities'; | ||
|
|
||
| export interface IChiclet { | ||
|
|
||
| } | ||
|
|
||
| export interface IChicletProps extends React.Props<Chiclet> { | ||
| /** | ||
| * Props to render in the chosen ChicletCard | ||
| */ | ||
| chicletCardProps?: IChicletCardProps | undefined; | ||
|
|
||
| /** @todo: description */ | ||
| getStyles?: IStyleFunction<IChicletCardStyleProps, IChicletCardStyles>; | ||
|
|
||
| /** | ||
| * Chiclet size to render | ||
| */ | ||
| size?: ChicletSize; | ||
|
|
||
| /** | ||
| * Footer to render for the component. | ||
| */ | ||
| footer?: React.ReactElement<any>; | ||
| } | ||
|
|
||
| export enum ChicletSize { | ||
| /** | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You likely don't need to supply values for this enum in its current usage. You may need it if you use the values later as part of assigning the sizes to the props for the node. I left a comment elsewhere about that potential enhancement. |
||
| * X-Small Chiclet | ||
| */ | ||
| xsmall = 0, | ||
|
|
||
| /** | ||
| * Small Chiclet | ||
| */ | ||
| small = 1, | ||
|
|
||
| /** | ||
| * Medium Chiclet | ||
| */ | ||
| medium = 2, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The naming convention for enum members is InitialCaps
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most enums in the fabric codebase seem to be lowerCamelCased... let's go with consistency now, and if we want to change the convention, do it all in one go? |
||
|
|
||
| /** | ||
| * Large Chiclet | ||
| */ | ||
| large = 3 | ||
| } | ||
|
|
||
| export interface IChicletStyleProps { | ||
| /** | ||
| * Theme for the component. | ||
| */ | ||
| theme?: ITheme; | ||
| } | ||
|
|
||
| export interface IChicletStyles { | ||
| root?: IStyle; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| import * as React from 'react'; | ||
| import { | ||
| BaseComponent, | ||
| css, | ||
| customizable, | ||
| classNamesFunction | ||
| } from '../../Utilities'; | ||
| import { IChicletCardStyles, IChicletCardStyleProps, IChicletCardProps } from './ChicletCard.types'; | ||
| import { mergeStyles } from '../../Styling'; | ||
| import { Icon } from 'office-ui-fabric-react/lib/Icon'; | ||
| import { Image } from 'office-ui-fabric-react/lib/Image'; | ||
| import { TestImages } from 'office-ui-fabric-react/src/common/TestImages'; | ||
|
|
||
| const getClassNames = classNamesFunction<IChicletCardStyleProps, IChicletCardStyles>(); | ||
|
|
||
| @customizable('ChicletCardBase', ['theme']) | ||
| export class ChicletCardBase extends BaseComponent<IChicletCardProps, any> { | ||
| private _classNames: { [key in keyof IChicletCardStyles]: string }; | ||
|
|
||
| public render() { | ||
| const { title, ogType, description, image, imageType, imageWidth, imageHeight, imageAlt, url, onClick, className, footer, theme, getStyles } = this.props; | ||
| const actionable = (onClick) ? true : false; | ||
|
|
||
| this._classNames = getClassNames(getStyles, { theme: theme! }); | ||
|
|
||
| // if this element is actionable it should have an aria role | ||
| const role = actionable ? (onClick ? 'button' : 'link') : undefined; | ||
| const tabIndex = actionable ? 0 : undefined; | ||
|
|
||
| var preview = this._renderPreviewImage(image, imageHeight, imageWidth, ogType); | ||
|
|
||
| return ( | ||
| <div | ||
| tabIndex={ tabIndex } | ||
| role={ role } | ||
| onClick={ actionable ? this._onClick : undefined } | ||
| className={ | ||
| css('ms-ChicletCard', className, mergeStyles(this._classNames.root)) } | ||
| > | ||
| <div | ||
| className={ mergeStyles(this._classNames.preview) } | ||
| > | ||
| { image ? | ||
| preview : | ||
| (<Image | ||
| src={ TestImages.documentPreviewTwo } | ||
| role='presentation' | ||
| alt='' | ||
| />) | ||
| } | ||
| </div> | ||
| <div | ||
| className={ mergeStyles(this._classNames.info) } | ||
| > | ||
| <div | ||
| className={ mergeStyles(this._classNames.title) } | ||
| > | ||
| { title ? title : "Placeholder" } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really want hardcoded english text as the default? if title is an optional prop, we should be able to render a ChicletCard without one. |
||
| </div> | ||
| <div | ||
| className={ mergeStyles(this._classNames.link) } | ||
| > | ||
| { url ? url : "https://onedrive.com/files/v-lygi/39192908430" } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Should we have a hard-wired product URL in the source code?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope! Good catch. It was a placeholder for my testing purposes, so I'll remove it. |
||
| </div> | ||
| { footer } | ||
| </div> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| private _renderPreviewImage(imageUrl?: string, imageHeight?: string, imageWidth?: string, ogType?: string, imageAlt?: string): React.ReactElement<React.HTMLAttributes<HTMLDivElement>> { | ||
| const image = ( | ||
| <Image | ||
| width={ imageWidth } | ||
| height={ imageHeight } | ||
| src={ TestImages.documentPreview } | ||
| role='presentation' | ||
| alt={ imageAlt ? imageAlt : undefined } | ||
| /> | ||
| ); | ||
|
|
||
| let icon; | ||
| switch (ogType) { | ||
| case "word": | ||
| icon = <Icon className={ mergeStyles(this._classNames.icon) } iconName='WordDocument' />; | ||
| break; | ||
| case "powerpoint": | ||
| icon = <Icon className={ mergeStyles(this._classNames.icon) } iconName='PowerPointDocument' />; | ||
| break; | ||
| case "excel": | ||
| icon = <Icon className={ mergeStyles(this._classNames.icon) } iconName='ExcelDocument' />; | ||
| break; | ||
| } | ||
|
|
||
| return ( | ||
| <div> | ||
| { image } | ||
| { icon } | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| private _onClick = (ev: React.MouseEvent<HTMLElement>): void => { | ||
| const { onClick } = this.props; | ||
| if (onClick) { | ||
| onClick(ev); | ||
| } | ||
| } | ||
|
|
||
| } | ||
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.
BaseChicletBase is a somewhat odd naming pattern. Is this common for OUIFR?
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.
Convention is that the base implementation (without styling) should only have the
.base.extension on it and not haveBasein the naming. (Exception for a few components that have not yet been converted over to Javascript styling.)I agree that
BaseChiclet.base.tsxis odd and I feel this should be namedChiclet.base.tsx. But I am also confused by the presence ofChiclet.base.tsxelsewhere in this PR and do not know what the difference is between that file and this one.What I'd like to recommend is renaming all of these files to
Chiclet.*.tsx, consistent with other components. If there is a specialized version of Chiclet, it can have its naming changed accordingly.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 agree that the naming has gotten a little odd/confusing haha. Here is how I have the Chiclet structured:
BaseChicletreturns a styledBaseChicletBase, which knows how to extract the relevant open graph meta tags to pass as props into theChiclet.Chicletreturns a styledChicletBase, which picks what sizeChicletwe want based on what ourBaseChicletpassed in, and passes that as a prop intoChicletCard-- we will be adding different kinds of Chiclets later on.ChicletCardreturns a styledChicletCardBase, which is really does most of the work for the overall construction of the the Chiclet component.One idea I have is to rename the
BaseChiclettoChicletand theChiclettoChicletPicker. @aditima do you have something else in mind that might be better?Additionally, I'm not sure that anything other than the
ChicletCardreally needs styling. So theBaseChicletandChicletmay not need these *.base.tsx files. @dzearing what do you think?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.
how about we rename