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.

Sign up for GitHub

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

2548: Refactoring #3012

Closed
wants to merge 2 commits into from
Closed

2548: Refactoring #3012

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
20fded0
2548: Refactoring
LeandraH Dec 2, 2024
e72b3cc
2548: More refactoring
LeandraH Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions web/src/components/CategoriesToolbar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import config from 'translations/src/config'
import { PdfIcon } from '../assets'
import { cmsApiBaseUrl } from '../constants/urls'
import CityContentToolbar from './CityContentToolbar'
import ToolbarItem from './ToolbarItem'
import ToolbarIcon from './ToolbarIcon'
Copy link
Member

@steffenkleinle steffenkleinle Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🤔 Hmm, I sadly don't like this new naming as it misses the point of what the component does. It is not just an icon, however, it is an item (or button). So please revert (to ToolbarItem or change to ToolbarItems if you want to keep the current behavior of having both items rendered in one component.
However, as mentioned in the first PR (#3004 (comment)), I'd prefer only having it render one toolbar item, see #3015 for a proposal on specifics. I think this would be a clearer separation of concerns and more intuitive on what a specific component does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, that is clearer! Do you want to continue in your PR or should I take over?

Copy link
Member

Choose a reason for hiding this comment

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

Totally up to you :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Go for it then, I'll close this one :)


type CategoriesToolbarProps = {
category?: CategoryModel
Expand All @@ -31,7 +31,7 @@ const CategoriesToolbar = (props: CategoriesToolbarProps): ReactElement => {
route={CATEGORIES_ROUTE}
feedbackTarget={category && !category.isRoot() ? category.slug : undefined}
pageTitle={pageTitle}>
<ToolbarItem icon={PdfIcon} text={t('createPdf')} to={pdfUrl} isDisabled={config.hasRTLScript(languageCode)} />
<ToolbarIcon icon={PdfIcon} text={t('createPdf')} to={pdfUrl} isDisabled={config.hasRTLScript(languageCode)} />
</CityContentToolbar>
)
}
Expand Down
8 changes: 4 additions & 4 deletions web/src/components/CityContentToolbar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ import { useTheme } from 'styled-components'
import { CopyIcon, DoneIcon } from '../assets'
import useWindowDimensions from '../hooks/useWindowDimensions'
import { RouteType } from '../routes'
import FeedbackToolbarItem from './FeedbackToolbarItem'
import FeedbackToolbarIcons from './FeedbackToolbarIcons'
import SharingPopup from './SharingPopup'
import Toolbar from './Toolbar'
import ToolbarItem from './ToolbarItem'
import ToolbarIcon from './ToolbarIcon'
import Tooltip from './base/Tooltip'

type CityContentToolbarProps = {
Expand Down Expand Up @@ -66,14 +66,14 @@ const CityContentToolbar = (props: CityContentToolbarProps) => {
isOpen={linkCopied}
place={tooltipDirection}
tooltipContent={t('common:copied')}>
<ToolbarItem
<ToolbarIcon
icon={linkCopied ? DoneIcon : CopyIcon}
text={t('copyUrl')}
onClick={copyToClipboard}
id='copy-icon'
/>
</Tooltip>
{hasFeedbackOption && <FeedbackToolbarItem route={route} slug={feedbackTarget} />}
{hasFeedbackOption && <FeedbackToolbarIcons route={route} slug={feedbackTarget} />}
</Toolbar>
)
}
Expand Down
12 changes: 6 additions & 6 deletions web/src/components/FeedbackToolbarItem.tsx → web/src/components/FeedbackToolbarIcons.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ import useCityContentParams from '../hooks/useCityContentParams'
import { RouteType } from '../routes'
import FeedbackContainer from './FeedbackContainer'
import Modal from './Modal'
import ToolbarItem from './ToolbarItem'
import ToolbarIcon from './ToolbarIcon'

type FeedbackToolbarItemProps = {
type FeedbackToolbarIconsProps = {
route: RouteType
slug?: string
}

const FeedbackToolbarItem = ({ route, slug }: FeedbackToolbarItemProps): ReactElement => {
const FeedbackToolbarIcons = ({ route, slug }: FeedbackToolbarIconsProps): ReactElement => {
const { cityCode, languageCode } = useCityContentParams()
const [isFeedbackOpen, setIsFeedbackOpen] = useState(false)
const [isSubmitted, setIsSubmitted] = useState(false)
Expand All @@ -39,15 +39,15 @@ const FeedbackToolbarItem = ({ route, slug }: FeedbackToolbarItemProps): ReactEl
/>
</Modal>
)}
<ToolbarItem
<ToolbarIcon
icon={HappySmileyIcon}
text={t('useful')}
onClick={() => {
setIsFeedbackOpen(true)
setIsPositiveRating(true)
}}
/>
<ToolbarItem
<ToolbarIcon
icon={SadSmileyIcon}
text={t('notUseful')}
onClick={() => {
Expand All @@ -59,4 +59,4 @@ const FeedbackToolbarItem = ({ route, slug }: FeedbackToolbarItemProps): ReactEl
)
}

export default FeedbackToolbarItem
export default FeedbackToolbarIcons
4 changes: 2 additions & 2 deletions web/src/components/SharingPopup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import styled, { css, useTheme } from 'styled-components'
import { CloseIcon, FacebookIcon, MailIcon, ShareIcon, WhatsappIcon } from '../assets'
import useWindowDimensions from '../hooks/useWindowDimensions'
import Portal from './Portal'
import ToolbarItem from './ToolbarItem'
import ToolbarIcon from './ToolbarIcon'
import Button from './base/Button'
import Icon from './base/Icon'
import Link from './base/Link'
Expand Down Expand Up @@ -249,7 +249,7 @@ const SharingPopup = ({ shareUrl, title, flow, portalNeeded }: SharingPopupProps
</TooltipContainer>
</>
)}
<ToolbarItem icon={ShareIcon} text={t('layout:share')} onClick={() => setShareOptionsVisible(true)} />
<ToolbarIcon icon={ShareIcon} text={t('layout:share')} onClick={() => setShareOptionsVisible(true)} />
</SharingPopupContainer>
)
}
Expand Down
20 changes: 10 additions & 10 deletions web/src/components/ToolbarItem.tsx → web/src/components/ToolbarIcon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import Icon from './base/Icon'
import Link from './base/Link'
import Tooltip from './base/Tooltip'

const StyledToolbarItem = styled(Link)<{ disabled?: boolean }>`
const StyledToolbarIcon = styled(Link)<{ disabled?: boolean }>`
display: inline-block;
padding: 8px;
border: none;
Expand All @@ -31,7 +31,7 @@ const StyledTooltip = styled(Tooltip)`
max-width: 250px;
`

type ItemProps =
type IconProps =
| {
onClick: () => void
to?: undefined
Expand All @@ -41,29 +41,29 @@ type ItemProps =
to: string
}

type ToolbarItemProps = {
type ToolbarIconProps = {
icon: string
text: string
id?: string
isDisabled?: boolean
} & ItemProps
} & IconProps

const ToolbarItem = ({ to, text, icon, isDisabled = false, onClick, id }: ToolbarItemProps): ReactElement => {
const ToolbarIcon = ({ to, text, icon, isDisabled = false, onClick, id }: ToolbarIconProps): ReactElement => {
const { t } = useTranslation('categories')
const toolTipId = spacesToDashes(text)
if (isDisabled) {
return (
<StyledTooltip id={toolTipId} tooltipContent={t('disabledPdf')}>
{/* @ts-expect-error wrong types from polymorphic 'as', see https://github.com/styled-components/styled-components/issues/4112 */}
<StyledToolbarItem as='div' id={id} label={text} disabled>
<StyledToolbarIcon as='div' id={id} label={text} disabled>
<StyledIcon src={icon} disabled />
<StyledSmallViewTip>{text}</StyledSmallViewTip>
</StyledToolbarItem>
</StyledToolbarIcon>
</StyledTooltip>
)
}
return (
<StyledToolbarItem
<StyledToolbarIcon
as={onClick ? Button : undefined}
id={id}
// @ts-expect-error wrong types from polymorphic 'as', see https://github.com/styled-components/styled-components/issues/4112
Expand All @@ -73,8 +73,8 @@ const ToolbarItem = ({ to, text, icon, isDisabled = false, onClick, id }: Toolba
disabled={false}>
<StyledIcon src={icon} disabled={false} />
<StyledSmallViewTip>{text}</StyledSmallViewTip>
</StyledToolbarItem>
</StyledToolbarIcon>
)
}

export default ToolbarItem
export default ToolbarIcon
6 changes: 3 additions & 3 deletions web/src/components/__tests__/FeedbackToolbarItem.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import React, { ReactElement } from 'react'
import { CATEGORIES_ROUTE } from 'shared'

import { renderWithRouterAndTheme } from '../../testing/render'
import FeedbackToolbarItem from '../FeedbackToolbarItem'
import FeedbackToolbarIcons from '../FeedbackToolbarIcons'

jest.mock('react-i18next')
jest.mock('shared/api', () => ({
Expand All @@ -15,10 +15,10 @@ jest.mock('shared/api', () => ({
}))
jest.mock('focus-trap-react', () => ({ children }: { children: ReactElement }) => <div>{children}</div>)

describe('FeedbackToolbarItem', () => {
describe('FeedbackToolbarIcons', () => {
it('should open and update title on submit feedback', async () => {
const { queryByText, findByText, getByText } = renderWithRouterAndTheme(
<FeedbackToolbarItem route={CATEGORIES_ROUTE} slug='my-slug' />,
<FeedbackToolbarIcons route={CATEGORIES_ROUTE} slug='my-slug' />,
)

expect(queryByText('feedback:headline')).toBeFalsy()
Expand Down
Loading