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

Rename bp-table -> pt-table. Upgrade script updates #1906

Merged
merged 17 commits into from
Dec 14, 2017

Conversation

themadcreator
Copy link
Contributor

@themadcreator themadcreator commented Dec 13, 2017

Add rename for all classname strings bp-table -> pt-table
Change onFocus from rename to warning since it's likely to clobber common uses
Add other core renames to script

@blueprint-bot
Copy link

Run script over packages

Preview: documentation | table

@@ -35,7 +35,7 @@

// CSS examples

#{reference("pt-file-upload")},
#{reference("pt-file-input")},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this one missed from #1880 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it. Good thing we have a script!

@blueprint-bot
Copy link

Add features page back to dev preview dist

Preview: documentation | table

@blueprint-bot
Copy link

Merge remote-tracking branch 'origin/master' into bd/rename-bp-classes

Preview: documentation | table

@blueprint-bot
Copy link

Fix features preview

Preview: documentation | landing | table

@cmslewis
Copy link
Contributor

cmslewis commented Dec 13, 2017

@themadcreator @adidahiya @giladgray @llorca

I really don't like that pt-table is a totally separate component from pt-table-X. It's innately confusing, and it's such a weird exception to our library-wide convention that every pt-[component]-X class is a child, variant, or modifier of pt-[component].

Ideas:

  • Rename <Table> to <SomethingElse> (e.g. <DataGrid>).
  • Introduce a top-level pt-tablejs or pt-table-js class, and change all packages/table CSS classes to use this prefix.
  • Rename the existing pt-table to pt-table-html (consistent with the "Table (HTML)" title in the docs).
  • Do nothing; live with the awkwardness.

@blueprint-bot
Copy link

Just kidding -- include CSS

Preview: documentation | table

@blueprint-bot
Copy link

Rename pt-table -> pt-html-table

Preview: documentation | table

@@ -7,7 +7,7 @@ Color aliases
These variables are semantic aliases of our [colors](#colors). They are used throughout Blueprint
itself to ensure consistent color usage across components.

<table class=pt-table>
<table class=pt-html-table>
Copy link
Contributor

Choose a reason for hiding this comment

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

missing quotes. though it seems to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

def works. let's add them for good measure

@@ -156,7 +156,7 @@ export const TAB_LIST = "pt-tab-list";
export const TAB_PANEL = "pt-tab-panel";
export const TABS = "pt-tabs";

export const TABLE = "pt-table";
export const TABLE = "pt-html-table";
Copy link
Contributor

Choose a reason for hiding this comment

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

should we rename constant too, to match?

}

function renameString() {
# Don't add word boundary so that partial css classnames rename
Copy link
Contributor

Choose a reason for hiding this comment

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

invalid comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yeah this is invalid

}

function renamePartialClass() {
# Don't add word boundary so that partial css classnames rename
Copy link
Contributor

Choose a reason for hiding this comment

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

invalid comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, this is still relevant, notice how we don't add the the \\b to this one.

@blueprint-bot
Copy link

renderName -> nameRenderer. Review docs, fix old references

Preview: documentation | table

@blueprint-bot
Copy link

pt-striped -> pt-html-table-striped. pt-borderer -> pt-html-table-bordered. script comment

Preview: documentation | table

@blueprint-bot
Copy link

Add other prefixes to rename script for protected abstract members

Preview: documentation | table

rowHeaderCellRenderer={this.renderRowHeader}
rowHeaderRef={this.refHandlers.rowHeader}
scrollContainerRef={this.refHandlers.scrollContainer}
/>
<div className={classNames(Classes.TABLE_OVERLAY_LAYER, "bp-table-reordering-cursor-overlay")} />
<div className={classNames(Classes.TABLE_OVERLAY_LAYER, "pt-table-reordering-cursor-overlay")} />
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we should make this a constant in Classes

@blueprint-bot
Copy link

Extract const

Preview: documentation | 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.

merge when CI passes

@blueprint-bot
Copy link

Merge remote-tracking branch 'refs/remotes/origin/master' into bd/rename-bp-classes

Preview: documentation | table

@themadcreator themadcreator merged commit eb09739 into master Dec 14, 2017
@themadcreator themadcreator deleted the bd/rename-bp-classes branch December 14, 2017 20:13
@giladgray giladgray mentioned this pull request Jan 9, 2018
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants