-
Notifications
You must be signed in to change notification settings - Fork 271
feat(control-utils): add infotooltipwithtrigger #442
Changes from 6 commits
5dc5d23
5efe029
9acf813
bf1296b
00561e3
ed63e91
3fa07d2
dc8cb7e
7f27efe
694c63d
8cac2d1
c0ce659
1c4be62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
/** | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
import React from 'react'; | ||
import { kebabCase } from 'lodash'; | ||
// @ts-ignore | ||
import { Tooltip, OverlayTrigger } from 'react-bootstrap'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer to remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I remember correctly, bootstrap 3 doesn't have types support. We ran into this problem in incubator-superset as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of @ts-ignore, you can add a declaration file. Even if it is an empty declaration There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! That means we can add types for bootstrap back in incubator-superset too There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ktmud I tried adding the types for bootstrap, but that leads to an issue in CI. It looks like our version of typescript isn't happy with those type definitions, any ideas on how to fix? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, should've used Btw, it's preferable to have |
||
|
||
const defaultProps = { | ||
className: 'text-muted', | ||
icon: 'info-circle', | ||
placement: 'right', | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer to use ES6 function argument defaults. |
||
const tooltipStyle = { wordWrap: 'break-word' } as any; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Try avoid const tooltipStyle: React.CSSProperties = { wordWrap: 'break-word' }; Since this rule is so simple, you can probably also just inline the style in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. having constant here is better so react won't re-render. when passing object to |
||
|
||
interface Props { | ||
label: string; | ||
tooltip: string; | ||
icon: string; | ||
onClick: () => void; | ||
placement: string; | ||
bsStyle: string; | ||
className: string; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make those with defaults optional. interface Props {
label: string;
icon: string;
tooltip?: string;
onClick?: () => void;
placement?: string;
bsStyle?: string;
className?: string;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 @ktmud with the small change to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
export default function InfoTooltipWithTrigger({ | ||
kristw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
label, | ||
tooltip, | ||
icon, | ||
className, | ||
onClick, | ||
placement, | ||
bsStyle, | ||
}: Props) { | ||
const iconClass = `fa fa-${icon} ${className} ${bsStyle ? `text-${bsStyle}` : ''}`; | ||
const onKeyPress = (event: React.KeyboardEvent) => { | ||
if (event.key === 'Enter' || event.key === 'space') { | ||
onClick(); | ||
} | ||
}; | ||
const iconEl = ( | ||
<i | ||
role="button" | ||
tabIndex={0} | ||
className={iconClass} | ||
style={{ cursor: onClick ? 'pointer' : undefined }} | ||
onClick={onClick} | ||
onKeyPress={onClick && onKeyPress} | ||
/> | ||
); | ||
if (!tooltip) { | ||
return iconEl; | ||
} | ||
return ( | ||
<OverlayTrigger | ||
placement={placement as any} | ||
overlay={ | ||
<Tooltip id={`${kebabCase(label)}-tooltip`} style={tooltipStyle}> | ||
{tooltip} | ||
</Tooltip> | ||
} | ||
> | ||
{iconEl} | ||
</OverlayTrigger> | ||
); | ||
} | ||
|
||
InfoTooltipWithTrigger.defaultProps = defaultProps; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: dependencies
peerDependencies
, as they are shared by multiplesuperset-ui
packages and should be upgraded all at once.react-bootstrap@1
yet. It's for Bootstrap v4, but we are still stuck with Boostrap v3. Checkincubator-superset
for property versions forreact-bootstrap
and@types/react-bootstrap
.lodash
? IskebabCase
absolutely necessary? Could you investigate why do we needed an ID for the tooltip in the first place? It's the best if we could just remove it. Less dependencies the better.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding 3, I think that kind of effort is better left until after we've moved things from superset. This should be as clean a refactor as possible, just moving code into superset-ui.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
react
in particular should always bepeerDependencies
in any library. There should always be onereact
.react-bootstrap
can be left independencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't agree with making these peer dependencies, because the code directly depends on them. Peer dependencies aren't meant to be used for optimization, npm does that automatically when version numbers of dependencies are compatible with each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with making them
dependencies
as-is exceptreact
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, npm might be able to remove duplicates, but I think there is also the implication of what if it is not compatible? Are we really OK with having multiple versions of the dependent libraries in our app?
These
superset-ui
packages cannot work outside of Superset or Superset UI demos, so they should not introduce their own large non-optional dependencies. UsingpeerDependencies
kind of forces package maintainers and library users to keep the dependencies up to date.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is how all packages work tho. I am ok having multiple versions of the dependent instead of worrying about breaking anything if bump version of one dependencies.
This is actually not entirely correct. These packages can work outside of Superset and Superset UI demos. They are independent packages on their own (number format, time format, color, etc.) which do not need Superset to function. I have them as dependencies in other projects and have always think of them independent from
incubator-superset
. They should have their own dependencies.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All other superset-ui packages have things (including lodash) listed under
dependencies
. Is this different somehow?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am proposing we proceed with moving
react
topeerDependencies
and leave the remaining independencies
unless there is objection?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No objection here. I actually already moved react, but haven't pushed it yet.