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

components for generic HTML elements #2456

Merged
merged 18 commits into from
May 7, 2018
Merged

components for generic HTML elements #2456

merged 18 commits into from
May 7, 2018

Conversation

giladgray
Copy link
Contributor

@giladgray giladgray commented May 3, 2018

  • add React components for HTML elements that have classes:
    • H1-6, Blockquote, Code, Pre, Ul, Ol, Table
  • add blueprint-html-components TSLint rule
  • 🔥 remove Classes.HOTKEY_GROUP

@giladgray giladgray requested a review from jkillian May 3, 2018 19:01

export class Table extends React.PureComponent<ITableHtmlProps> {
public render() {
const { bordered, className, elementRef, interactive, small, striped, ...htmlProps } = this.props;
Copy link
Contributor

Choose a reason for hiding this comment

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

className should be passed to classes down below

public static metadata: Lint.IRuleMetadata = {
ruleName: "blueprint-html-components",
// tslint:disable-next-line:object-literal-sort-keys
description: "Enforce usage of Blueprint components over JSX intrinsic elements.",
Copy link
Contributor

@jkillian jkillian May 3, 2018

Choose a reason for hiding this comment

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

probably worth adding a rationale here explaining why in more details (metadata supports an actual rationale field)

Copy link
Contributor

Choose a reason for hiding this comment

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

or just have a docs page with an explanation for the rules

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you already did this actually, I skipped that file haha

@@ -0,0 +1,5 @@
<h1>Title</h1>
Copy link
Contributor

Choose a reason for hiding this comment

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

don't really need a test case for when the rule is disabled


<H3>Subtitle</H3>
<Pre>block</Pre>

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe test some self closing tags too just for the heck of it

- `Pre`
- `Ol`
- `Ul`
- `Table` (see [Table (HTML)](http://localhost:9000/#core/components/table-html))
Copy link
Contributor

Choose a reason for hiding this comment

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

Controversial proposal: I know we're mimicking HTML here, but since this is React stuff, what do you think about Heading1...6, OrderedList, UnorderedList...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uppercase in docs

@blueprint-bot
Copy link

remove false lint test

Preview: documentation | landing | table

@blueprint-bot
Copy link

uppercase UL/OL in docs

Preview: documentation | landing | table

@blueprint-bot
Copy link

fix more lint

Preview: documentation | landing | table

@blueprint-bot
Copy link

exclude Table from coverage

Preview: documentation | landing | table

@@ -57,11 +58,7 @@ export class Hotkeys extends AbstractPureComponent<IHotkeysProps, {}> {
for (const hotkey of hotkeys) {
const groupLabel = hotkey.group;
if (groupLabel !== lastGroup) {
elems.push(
<h4 key={`group-${elems.length}`} className={Classes.HOTKEY_GROUP}>
Copy link
Contributor

Choose a reason for hiding this comment

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

removing this class name might break people's selectors, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes true it might, but it's been replaced with HEADING

@blueprint-bot
Copy link

remove extra space

Preview: documentation | landing | table

@blueprint-bot
Copy link

delete Classes.HOTKEY_GROUP

Preview: documentation | landing | table

@giladgray giladgray merged commit 7ba0dae into develop May 7, 2018
@giladgray giladgray deleted the gg/html-components branch May 7, 2018 19:23
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.

4 participants