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.

Sign up for GitHub

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

Jump to bottom

Alignment enum #2089

Merged
merged 5 commits into from
Feb 6, 2018
Merged

Alignment enum #2089

merged 5 commits into from
Feb 6, 2018

Conversation

giladgray
Copy link
Contributor

  • add Alignment enum and Classes.alignmentClass()

@giladgray giladgray requested a review from adidahiya February 5, 2018 23:23
@blueprint-bot
Copy link

docs about Alignment enum

Preview: documentation | landing | table

@blueprint-bot
Copy link

copyright, nits

Preview: documentation | landing | table

adidahiya
adidahiya suggested changes Feb 6, 2018
View reviewed changes
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.

this is wonky

packages/core/src/components/button/abstractButton.tsx Outdated
@@ -26,6 +26,8 @@ export interface IButtonProps extends IActionProps {
* Text alignment within button. By default, icons and text will be centered within the button.
* Passing this prop will cause the text container to fill the button and align the text within that
* to the appropriate side. `icon` and `rightIcon` will be pushed to either side.
*
* The `Alignment` enum provides constants for allowed values.
Copy link
Contributor

Choose a reason for hiding this comment

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

then why not use the enum here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like #2090, i want to show the actual options in the docs, not the type alias name.

packages/core/src/components/button/buttonGroup.tsx Outdated
@@ -15,6 +15,8 @@ export interface IButtonGroupProps extends IProps, React.HTMLProps<HTMLDivElemen
* `align="left"` will left-align button text and push `rightIcon` to right side.
* `align="right"` right-aligns text and pushes `icon` to left side.
* This prop only has an effect if buttons are wider than their default widths.
*
* The `Alignment` enum provides constants for allowed values.
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

string literals are not assignable to enum types (we learned this with string enums #1921)

packages/core/src/components/navbar/navbarGroup.tsx
@@ -12,6 +12,7 @@ import { IProps } from "../../common/props";
export interface INavbarGroupProps extends React.HTMLProps<HTMLDivElement>, IProps {
/**
* The side of the navbar on which the group should appear.
* The `Alignment` enum provides constants for these values.
Copy link
Contributor

Choose a reason for hiding this comment

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

same

packages/core/src/common/alignment.ts Outdated
* Licensed under the terms of the LICENSE file distributed with this project.
*/

export type AlignmentType = "center" | "left" | "right";
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need this in addition to the enum?

@giladgray
Copy link
Contributor Author

giladgray commented Feb 6, 2018 •
edited
Loading

@adidahiya we learned in #1921 that "primary" is not assignable to enum type Intent so I decided to try a different approach to typing a feature like that. hence the separation between enum and union type. i created union type for use internally, but decided not to use it for public component props so the docs will show the three accepted values instead of the inscrutable AlignmentType.

let me know how you'd like to proceed.

@adidahiya
Copy link
Contributor

If you really want both a type and an enum, you have to do something like this:

export type Alignment = "left" | "center" | "right";
export const Alignment = {
    LEFT: "left" as Alignment,
    CENTER: "center" as Alignment,
    RIGHT: "right" as ALIGNMENT,
};

... so we should either do that (overload one symbol) or do nothing at all

@giladgray
Copy link
Contributor Author

I think you mean as "left", etc.

@giladgray
Copy link
Contributor Author

@adidahiya hmm yeah thoughts on migrating Position and Intent to work like that?

@blueprint-bot
Copy link

const/type Alignment allows string literal or constant usage

Preview: documentation | landing | table

@adidahiya adidahiya merged commit e52f20f into develop Feb 6, 2018
@adidahiya adidahiya deleted the gg/alingment branch February 6, 2018 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adidahiya adidahiya adidahiya approved these changes

@llorca llorca llorca approved these changes

Assignees
No one assigned
Labels
Package: core Type: refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@giladgray @blueprint-bot @adidahiya @llorca