Skip to content
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

iconName => icon prop #2070

Merged
merged 14 commits into from
Feb 6, 2018
Merged

iconName => icon prop #2070

merged 14 commits into from
Feb 6, 2018

Conversation

giladgray
Copy link
Contributor

@giladgray giladgray commented Feb 2, 2018

Fixes #2066

Checklist

  • Update documentation

Changes proposed in this pull request:

  • Icon iconName?: IconName => icon?: LegacyIconName | JSX.Element | false | null | undefined
  • other cases get icon?: IconName | JSX.Element
    • leftIconName, rightIconName get the same treatment

@giladgray giladgray changed the title iconName => icon prop iconName => icon prop, short names only Feb 2, 2018
@blueprint-bot
Copy link

fix tests

Preview: documentation | landing | table

@blueprint-bot
Copy link

fix icons docs

Preview: documentation | landing | table

@blueprint-bot
Copy link

bring back LegacyIconName (one big break per PR)

Preview: documentation | landing | table

@adidahiya
Copy link
Contributor

adidahiya commented Feb 2, 2018

can you update the prop renaming script to include iconName -> icon?

@blueprint-bot
Copy link

semantic merge conflicts

Preview: documentation | landing | table

@blueprint-bot
Copy link

add iconName to rename script

Preview: documentation | landing | table

@blueprint-bot
Copy link

add iconName to rename script

Preview: documentation | landing | table

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

Can you add a docs example that demonstrates a custom SVG icon in a MenuItem (our original internal use case for this)?

*/
iconName?: LegacyIconName;
icon: LegacyIconName | JSX.Element | false | null | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is pretty unintuitive, so you should leave clearer documentation on why would a user want to use this with JSX.Element since it's just a pass-through render. the short answer is: they wouldn't. it's only there to make our codebase cleaner since <Icon> is used everywhere.

import { IconClasses } from "@blueprintjs/core";
<Icon iconName={IconClasses.ALIGN_LEFT} iconSize={Icon.SIZE_LARGE} />
<Icon iconName="globe" iconSize={20} />
<Icon iconName="graph" iconSize={Icon.SIZE_LARGE} intent={Intent.PRIMARY} />
Copy link
Contributor

Choose a reason for hiding this comment

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

need to update these usage docs


it("iconName=undefined renders nothing", () => {
const icon = shallow(<Icon iconName={undefined} />);
it("passes through icon element unchanged", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

also leave that documentation here, to explain why we're supporting this weird prop

@giladgray giladgray changed the title iconName => icon prop, short names only iconName => icon prop Feb 2, 2018
@@ -57,3 +59,12 @@ export class MenuExample extends BaseExample<{}> {
);
}
}

const PalantirLogo: React.SFC = () => (
<svg className={Classes.ICON} width="16" height="16" viewBox="0 0 18 23" xmlns="http://www.w3.org/2000/svg">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adidahiya maybe some props should be passed into given icon element?
especially if it's an svg element: className, iconSize, color.
or we could support a paths prop?

Copy link
Contributor

Choose a reason for hiding this comment

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

no, I'd rather not add that complexity to <Icon>

@blueprint-bot
Copy link

add custom svg icon item

Preview: documentation | landing | table

@blueprint-bot
Copy link

Merge branch 'develop' of github.com:palantir/blueprint into gg/icon-prop

Preview: documentation | landing | table

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

icons page is broken

@@ -57,3 +59,12 @@ export class MenuExample extends BaseExample<{}> {
);
}
}

const PalantirLogo: React.SFC = () => (
<svg className={Classes.ICON} width="16" height="16" viewBox="0 0 18 23" xmlns="http://www.w3.org/2000/svg">
Copy link
Contributor

Choose a reason for hiding this comment

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

no, I'd rather not add that complexity to <Icon>

@giladgray giladgray mentioned this pull request Feb 5, 2018
@giladgray
Copy link
Contributor Author

icons page fixed

@blueprint-bot
Copy link

bold icon names

Preview: documentation | landing | table

@adidahiya adidahiya merged commit d060250 into develop Feb 6, 2018
@adidahiya adidahiya deleted the gg/icon-prop branch February 6, 2018 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants