[Draft] Entity title #6278
[Draft] Entity title #6278
Conversation
| export const ENTITY_TITLE_TOP_LINE = `${ENTITY_TITLE}-top-line`; | ||
| export const ENTITY_TITLE_TAGS_CONTAINER = `${ENTITY_TITLE}-tags-container`; | ||
| export const ENTITY_TITLE_TITLE = `${ENTITY_TITLE}-title`; | ||
| export const ENTITY_TITLE_HAS_SUBTITLE = `${ENTITY_TITLE}-has-subtitle`; |
There was a problem hiding this comment.
it looks like this class is unused
| <Icon icon={icon} aria-hidden={true} tabIndex={-1} className={Classes.TEXT_MUTED} /> | ||
| </div> | ||
| )} | ||
| <div className={Classes.ENTITY_TITLE_RIGHT}> |
There was a problem hiding this comment.
"right" doesn't seem like the correct terminology here, it's more of the main content. I would call it "ENTITY_TITLE_TEXT" instead
| subtitle, | ||
| titleURL, | ||
| }) => { | ||
| const titleComponent = React.useMemo(() => { |
There was a problem hiding this comment.
this is of type JSX.Element, so it should be called:
| const titleComponent = React.useMemo(() => { | |
| const titleElement = React.useMemo(() => { |
|
|
||
| const titleClassName = classNames(Classes.ENTITY_TITLE_TITLE); | ||
|
|
||
| switch (headingSize) { |
There was a problem hiding this comment.
instead of this switch case syntax and exposing a new HeadingSize enum in the public API, I think it might be nicer to have a prop that accepts a function component:
<EntityTitle title="Foo" heading={H4} />
interface EntityTitleProps {
/**
* React component to render the main title heading. This defaults to Blueprint's `<Text>` component,
* which inherits font size from its containing element(s).
*
* To render larger, more prominent titles, Use Blueprint's heading components instead
* (e.g. `{ H1 } from "@blueprintjs/core"`).
*
* @default Text
*/
heading?: React.FC<any>;
}then you can simply render:
React.createElement(heading, { className: Classes.ENTITY_TITLE_TITLE }, maybeTitleWithURL);| } | ||
| }, [title, titleURL, headingSize]); | ||
|
|
||
| const subtitleComponent = React.useMemo(() => { |
There was a problem hiding this comment.
again, this is an element rather than a component value. actually you can drop the suffix altogether. it's more important to use the "maybe" prefix in this case:
| const subtitleComponent = React.useMemo(() => { | |
| const maybeSubtitle = React.useMemo(() => { |
| Classes.TEXT_MUTED, | ||
| headingSize == null && Classes.TEXT_SMALL, | ||
| Classes.ENTITY_TITLE_SUBTITLE, |
There was a problem hiding this comment.
use classNames object syntax for this logic instead of &&. also put the conditionally-applied classes last in the liast:
| Classes.TEXT_MUTED, | |
| headingSize == null && Classes.TEXT_SMALL, | |
| Classes.ENTITY_TITLE_SUBTITLE, | |
| Classes.ENTITY_TITLE_SUBTITLE, | |
| Classes.TEXT_MUTED, | |
| { [Classes.TEXT_SMALL]: headingSize == null }, |
| }, [headingSize, subtitle]); | ||
|
|
||
| return ( | ||
| <div className={classNames(className, Classes.ENTITY_TITLE, headingSize != null && css[headingSize])}> |
There was a problem hiding this comment.
we'll need to refactor this to work with my heading?: React.FC proposal above...
| </div> | ||
| )} | ||
| <div className={Classes.ENTITY_TITLE_RIGHT}> | ||
| <div className={Classes.ENTITY_TITLE_TOP_LINE}> |
There was a problem hiding this comment.
"top line" doesn't sound great, how about "ENTITY_TITLE_TITLE_AND_TAGS"
| EntityTitle.defaultProps = {}; | ||
| EntityTitle.displayName = `${DISPLAYNAME_PREFIX}.EntityTitle`; | ||
|
|
||
| type ISkeletonEntityTitleProps = Props & |
There was a problem hiding this comment.
we don't use I prefixes in types or interfaces names anymore. also we typically use interface instead of type to define props interfaces:
| type ISkeletonEntityTitleProps = Props & | |
| interface SkeletonEntityTitleProps extends Props { |
| hasSubtitle?: boolean; | ||
| }; | ||
|
|
||
| export const SkeletonEntityTitle: React.FC<ISkeletonEntityTitleProps> = ({ hasSubtitle, hasIcon, ...props }) => { |
There was a problem hiding this comment.
EntityTitleSkeleton would be a better name so that it gets grouped with EntityTitle in the imports list when we alphabetize imports.
Also, let's move this to its own file (typically keep one component per file)
|
superseded by #6590 |
No description provided.