msyavuz commented on code in PR #33090:
URL: https://github.com/apache/superset/pull/33090#discussion_r2054304825
##########
superset-frontend/src/components/DynamicEditableTitle/index.tsx:
##########
@@ -176,38 +166,28 @@ export const DynamicEditableTitle = memo(
showTooltip && currentTitle && !isEditing ? currentTitle : null
}
>
- {canEdit ? (
- <input
- data-test="editable-title-input"
- className="dynamic-title-input"
- aria-label={label ?? t('Title')}
- ref={contentRef}
- onChange={handleChange}
- onBlur={handleBlur}
- onClick={handleClick}
- onKeyPress={handleKeyPress}
- placeholder={placeholder}
- value={currentTitle}
- css={css`
- cursor: ${isEditing ? 'text' : 'pointer'};
-
- ${inputWidth &&
- inputWidth > 0 &&
- css`
- width: ${inputWidth + 1}px;
- `}
+ <Input
+ data-test="editable-title-input"
+ variant="borderless"
+ aria-label={label ?? t('Title')}
+ className="dynamic-title-input"
+ value={currentTitle}
+ onChange={handleChange}
+ onBlur={handleBlur}
+ onClick={handleClick}
+ onPressEnter={handleKeyPress}
+ placeholder={placeholder}
+ css={css`
+ font-size: ${theme.fontSizeXL}px;
+ transition: auto;
+ ${inputWidth &&
+ inputWidth > 0 &&
+ css`
+ width: ${inputWidth}px;
`}
- />
- ) : (
- <span
- className="dynamic-title"
- aria-label={label ?? t('Title')}
- ref={contentRef}
- data-test="editable-title"
- >
- {currentTitle}
- </span>
- )}
+ `}
+ disabled={!canEdit}
+ />
Review Comment:
This works as well but maybe we could use the `editable` prop on typography
elements for things like this?
https://ant.design/components/typography#typography-demo-editable
##########
superset-frontend/src/dashboard/components/EmbeddedModal/index.tsx:
##########
@@ -178,31 +180,41 @@ export const DashboardEmbedControls = ({ dashboardId,
onHide }: Props) => {
)}
<p>
Review Comment:
I know it is not a part of this pr but, should we use Typography for `<p>`
`<h1>` `<h2>` `<h3>` etc?
##########
superset-frontend/src/explore/components/EmbedCodeContent.jsx:
##########
@@ -90,13 +86,7 @@ const EmbedCodeContent = ({ formData, addDangerToast }) => {
<CopyToClipboard
shouldShowText={false}
text={html}
- copyNode={
- <CopyButtonEmbedCode buttonSize="xsmall">
- {/* TODO: Remove fa-icon */}
- {/* eslint-disable-next-line icons/no-fa-icons-usage */}
- <i className="fa fa-clipboard" />
- </CopyButtonEmbedCode>
- }
+ copyNode={<Icons.CopyOutlined />}
Review Comment:
We shouldn't lose the button here for accessibility.
##########
superset-frontend/src/explore/components/EmbedCodeContent.jsx:
##########
@@ -107,46 +97,42 @@ const EmbedCodeContent = ({ formData, addDangerToast }) =>
{
readOnly
css={theme => css`
resize: vertical;
+ margin-top: ${theme.sizeUnit * 2}px;
padding: ${theme.sizeUnit * 2}px;
font-size: ${theme.fontSizeSM}px;
border-radius: 4px;
background-color: ${theme.colorPrimaryBg};
`}
/>
</div>
- <div
+ <Space
+ direction="orizzontal"
css={theme => css`
- display: flex;
margin-top: ${theme.sizeUnit * 4}px;
- & > div {
- margin-right: ${theme.sizeUnit * 2}px;
- }
- & > div:last-of-type {
- margin-right: 0;
- margin-left: ${theme.sizeUnit * 2}px;
- }
`}
>
<div>
- <label htmlFor="embed-height">{t('Chart height')}</label>
+ <Typography.Text type="secondary">
+ {t('Chart height')}
+ </Typography.Text>
<Input
- type="text"
+ type="number"
defaultValue={height}
name="height"
onChange={handleInputChange}
/>
</div>
<div>
- <label htmlFor="embed-width">{t('Chart width')}</label>
+ <Typography.Text type="secondary">{t('Chart
width')}</Typography.Text>
<Input
- type="text"
+ type="number"
Review Comment:
I guess it is since we change it here as well.
##########
superset-frontend/src/dashboard/components/EmbeddedModal/index.tsx:
##########
@@ -178,31 +180,41 @@ export const DashboardEmbedControls = ({ dashboardId,
onHide }: Props) => {
)}
<p>
{t('For further instructions, consult the')}{' '}
- <a href={docsUrl} target="_blank" rel="noreferrer">
+ <Typography.Link href={docsUrl} target="_blank" rel="noreferrer">
{docsDescription
? docsDescription()
: t('Superset Embedded SDK documentation.')}
- </a>
+ </Typography.Link>
</p>
<h3>{t('Settings')}</h3>
- <FormItem>
- <label htmlFor="allowed-domains">
- {t('Allowed Domains (comma separated)')}{' '}
- <InfoTooltipWithTrigger
- tooltip={t(
- 'A list of domain names that can embed this dashboard. Leaving
this field empty will allow embedding from any domain.',
- )}
- />
- </label>
- <Input
+ <Form layout="vertical">
+ <FormItem
name="allowed-domains"
- id="allowed-domains"
- value={allowedDomains}
- placeholder="superset.example.com"
- onChange={event => setAllowedDomains(event.target.value)}
- />
- </FormItem>
- <ButtonRow>
+ label={
+ <span>
+ {t('Allowed Domains (comma separated)')}{' '}
+ <InfoTooltipWithTrigger
+ placement="top"
+ tooltip={t(
+ 'A list of domain names that can embed this dashboard.
Leaving this field empty will allow embedding from any domain.',
+ )}
+ />
+ </span>
+ }
+ >
+ <Input
+ id="allowed-domains"
+ value={allowedDomains}
+ placeholder="superset.example.com"
+ onChange={event => setAllowedDomains(event.target.value)}
+ />
+ </FormItem>
+ </Form>
+ <ButtonRow
+ css={theme => css`
+ margin-top: ${theme.sizeUnit * 4}px;
Review Comment:
We can use `theme.margin` here as well
##########
superset-frontend/src/explore/components/EmbedCodeContent.jsx:
##########
@@ -107,46 +97,42 @@ const EmbedCodeContent = ({ formData, addDangerToast }) =>
{
readOnly
css={theme => css`
resize: vertical;
+ margin-top: ${theme.sizeUnit * 2}px;
padding: ${theme.sizeUnit * 2}px;
font-size: ${theme.fontSizeSM}px;
border-radius: 4px;
background-color: ${theme.colorPrimaryBg};
`}
/>
</div>
- <div
+ <Space
+ direction="orizzontal"
Review Comment:
I guess this is Italian 😄 ?
```suggestion
direction="horizontal"
```
##########
superset-frontend/src/features/alerts/buildErrorTooltipMessage.tsx:
##########
@@ -38,11 +34,16 @@ export const buildErrorTooltipMessage = (
return (
<div>
{TRANSLATIONS.ERROR_TOOLTIP_MESSAGE}
- <StyledList>
- {sectionErrors.map(err => (
- <li key={err}>{err}</li>
- ))}
- </StyledList>
+ <List
+ dataSource={sectionErrors}
+ renderItem={err => (
+ <List.Item>
+ <Typography.Text>• {err}</Typography.Text>
Review Comment:
Is this `Typography.Text` here necessary?
##########
superset-frontend/src/components/Typography/index.tsx:
##########
@@ -16,6 +16,31 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { Typography } from 'antd-v5';
+import { styled, css } from '@superset-ui/core';
+import { Typography as AntdTypography } from 'antd-v5';
-export default Typography;
+const StyledLink = styled(AntdTypography.Link)`
+ ${({ theme }) =>
+ css`
+ && {
+ color: ${theme.colorLink};
+ &:hover {
+ color: ${theme.colorLinkHover};
+ }
+ `}
+ }
+`;
+
+export const Typography: typeof AntdTypography & {
+ Text: typeof AntdTypography.Text;
+ Link: typeof StyledLink;
+ Title: typeof AntdTypography.Title;
+ Paragraph: typeof AntdTypography.Paragraph;
+} = Object.assign(AntdTypography, {
+ Text: AntdTypography.Text,
+ Link: StyledLink,
+ Title: AntdTypography.Title,
+ Paragraph: AntdTypography.Paragraph,
+});
Review Comment:
AntdTypography should cover sub-components. Something like this would be
enough:
```suggestion
export const Typography: typeof AntdTypography =
Object.assign(AntdTypography, {
Text: AntdTypography.Text,
Link: StyledLink,
Title: AntdTypography.Title,
Paragraph: AntdTypography.Paragraph,
});
```
##########
superset-frontend/src/explore/components/EmbedCodeContent.jsx:
##########
@@ -107,46 +97,42 @@ const EmbedCodeContent = ({ formData, addDangerToast }) =>
{
readOnly
css={theme => css`
resize: vertical;
+ margin-top: ${theme.sizeUnit * 2}px;
padding: ${theme.sizeUnit * 2}px;
font-size: ${theme.fontSizeSM}px;
border-radius: 4px;
background-color: ${theme.colorPrimaryBg};
`}
/>
</div>
- <div
+ <Space
+ direction="orizzontal"
css={theme => css`
- display: flex;
margin-top: ${theme.sizeUnit * 4}px;
- & > div {
- margin-right: ${theme.sizeUnit * 2}px;
- }
- & > div:last-of-type {
- margin-right: 0;
- margin-left: ${theme.sizeUnit * 2}px;
- }
`}
>
<div>
- <label htmlFor="embed-height">{t('Chart height')}</label>
+ <Typography.Text type="secondary">
+ {t('Chart height')}
+ </Typography.Text>
<Input
- type="text"
+ type="number"
Review Comment:
Is this intentional?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
