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

506 update shared mutations page #529

Open
wants to merge 4 commits into
base: 505-update-plotting-pages
Choose a base branch
from
+162 −146
Open

506 update shared mutations page #529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
267d361
feat-506: make shared mutations page respond to pango switch
Feb 21, 2025
925fafb
feat-506: improve toggle layout
Feb 24, 2025
dae4890
feat-506: improve navbar layout when resizing the screen
Feb 24, 2025
9bccaca
feat-506: fix test
Feb 26, 2025
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: 1 addition & 3 deletions web/src/components/Common/LastUpdated.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,7 @@ export function LastUpdated({ className }: HTMLProps<HTMLParagraphElement>) {

return (
<LastUpdatedText className={classNames(className)}>
<span className="ms-1" title={full}>
{t('Last updated: {{ date }}', { date: date })}
</span>
<span title={full}>{t('Last updated: {{ date }}', { date: date })}</span>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<span title={full}>{t('Last updated: {{ date }}', { date: date })}</span>
<span title={full}>{t('Last updated: {{ date }}', { date })}</span>

Does this work als well?

</LastUpdatedText>
)
}
25 changes: 25 additions & 0 deletions web/src/components/Common/NomenclatureSwitch.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { useRecoilState } from 'recoil'
import React from 'react'
import { styled } from 'styled-components'
import { enablePangolinAtom } from 'src/state/Nomenclature'
import { ToggleTwoLabels } from 'src/components/Common/ToggleTwoLabels'

export function NomenclatureSwitch({ className }: { className?: string }) {
const [enablePangolin, setEnablePangolin] = useRecoilState(enablePangolinAtom)

return (
<NomenclatureToggle
className={className}
identifier="nomenclature-switch"
title="Switch nomenclature"
checked={enablePangolin}
onCheckedChanged={setEnablePangolin}
labelLeft="Pangolin"
labelRight="Nextstrain"
/>
)
}

const NomenclatureToggle = styled(ToggleTwoLabels)`
transform: scale(0.9);
`
19 changes: 9 additions & 10 deletions web/src/components/Common/ToggleTwoLabels.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import { StrictOmit } from 'ts-essentials'
export const Label = styled.label`
flex: 0;
display: flex;
margin: 0 auto;
gap: 0.25rem;
align-items: center;
word-wrap: normal;
text-overflow: clip;
white-space: nowrap;
Expand Down Expand Up @@ -72,15 +73,13 @@ export function ToggleTwoLabels({
return (
<Label htmlFor={identifier} className={className} title={title}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the htmlFor necessary? I thought this should already work, since the input element is inside the label?

{labelRight}
<span className="me-2 ms-2">
<ToggleTwoLabelsBase
id={identifier}
className="react-toggle-two-labels-custom"
icons={false}
onChange={onChange}
{...props}
/>
</span>
<ToggleTwoLabelsBase
id={identifier}
className="react-toggle-two-labels-custom"
icons={false}
onChange={onChange}
{...props}
/>
Comment on lines +76 to +82
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we use an extra libray for a switch? It seems that bootstrap already comes with a switch: https://getbootstrap.com/docs/5.1/forms/checks-radios/ . Then we would not need to style it our own and we have less trouble with the css.
GPT suggested using this: (I have not tested it, but it seems sensible)

<div className="d-flex align-items-center">
      <label className="d-flex align-items-center" htmlFor="flexSwitchCheckDefault">
        <span className="me-2">Label 1</span>
        <div className="form-check form-switch">
          <input
            className="form-check-input"
            type="checkbox"
            id="flexSwitchCheckDefault"
          />
        </div>
        <span className="ms-2">Label 2</span>
      </label>
    </div>

{labelLeft}
</Label>
)
Expand Down
4 changes: 2 additions & 2 deletions web/src/components/Layout/LanguageSwitcher.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@ import { getLocaleWithKey, Locale, localesArray } from 'src/i18n/i18n'

export type LanguageSwitcherProps = DropdownProps

export function LanguageSwitcher({ ...restProps }: LanguageSwitcherProps) {
export function LanguageSwitcher({ className, ...restProps }: LanguageSwitcherProps) {
const [currentLocale, setCurrentLocale] = useRecoilState(localeAtom)
const [dropdownOpen, setDropdownOpen] = useState(false)
const toggle = useCallback(() => setDropdownOpen((prevState) => !prevState), [])
const setLocaleLocal = useCallback((locale: Locale) => () => setCurrentLocale(locale.key), [setCurrentLocale])

return (
<Dropdown className="language-switcher" isOpen={dropdownOpen} toggle={toggle} {...restProps}>
<Dropdown className={`language-switcher ${className}`} isOpen={dropdownOpen} toggle={toggle} {...restProps}>
<DropdownToggle nav caret>
<LanguageSwitcherItem locale={currentLocale} />
</DropdownToggle>
Expand Down
113 changes: 29 additions & 84 deletions web/src/components/Layout/Layout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,37 @@
import React, { PropsWithChildren, Suspense } from 'react'
import React, { PropsWithChildren } from 'react'
import { styled } from 'styled-components'
import { Container as ContainerBase, Row, Col } from 'reactstrap'
import Image from 'next/image'
import { ErrorBoundary } from 'react-error-boundary'
import { useRecoilState } from 'recoil'
import { LastUpdated } from '../Common/LastUpdated'
import { NavigationBar } from './NavigationBar'
import { FooterContent } from './Footer'
import GisaidLogoPNG from 'src/assets/images/GISAID_logo.png'
import { ChristmasLightRope, Santa, Snowfall } from 'src/components/Common/Christmas'
import { ChangelogButton } from 'src/components/Common/ChangelogButton'
import { useTranslationSafe } from 'src/helpers/useTranslationSafe'
import { LinkExternal } from 'src/components/Link/LinkExternal'
import { FetchError } from 'src/components/Error/FetchError'
import { ToggleTwoLabels } from 'src/components/Common/ToggleTwoLabels'
import { enablePangolinAtom } from 'src/state/Nomenclature'

export function Layout({ children }: PropsWithChildren<LayoutProps>) {
return (
<Container fluid>
<HeaderRow className={'gx-0'}>
<HeaderCol>
<NavigationBar />
<ChristmasLightRope />
</HeaderCol>
</HeaderRow>
Comment on lines +11 to +16
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not this PR, but in the html in the end there is no <header>. It would be nice to have this to make it more accessible.


<MainContainer fluid>
<MainRow className={'gx-0 mt-4 mt-md-2'}>
<MainCol>{children}</MainCol>
</MainRow>
</MainContainer>

<FooterRow className={'gx-0'}>
<FooterCol>
<FooterContent />
</FooterCol>
</FooterRow>

<Snowfall />
<Santa />
</Container>
)
}

const Container = styled(ContainerBase)`
min-height: 100%;
Expand Down Expand Up @@ -60,78 +77,6 @@ const FooterCol = styled(Col)`
padding: 0;
`

const GisaidText = styled.small`
font-size: 0.9rem;
`

export interface LayoutProps {
wide?: boolean
}

function NomenclatureSwitch() {
const [enablePangolin, setEnablePangolin] = useRecoilState(enablePangolinAtom)

return (
<ToggleTwoLabels
identifier="nomenclature-switch"
title="Switch nomenclature"
checked={enablePangolin}
onCheckedChanged={setEnablePangolin}
labelLeft="Pangolin"
labelRight="Nextstrain"
/>
)
}

export function Layout({ children }: PropsWithChildren<LayoutProps>) {
const { t } = useTranslationSafe()

return (
<Container fluid>
<HeaderRow className={'gx-0'}>
<HeaderCol>
<NavigationBar />
<ChristmasLightRope />
</HeaderCol>
</HeaderRow>

<Row className="ms-3 mt-2 d-none d-md-block gx-0">
<Col className="d-flex">
<GisaidText className="d-flex me-auto">
<span className="me-1 align-self-center">{t('Enabled by data from {{ gisaid }}', { gisaid: '' })}</span>
<LinkExternal className="align-self-center" href="https://www.gisaid.org/" icon={null}>
<Image src={GisaidLogoPNG} alt="GISAID" height={27} width={73} />
</LinkExternal>
</GisaidText>

<div className="d-flex">
<NomenclatureSwitch />

<ChangelogButton className="d-flex ms-auto">
<ErrorBoundary FallbackComponent={FetchError}>
<Suspense>
<LastUpdated className="d-flex ms-auto" />
</Suspense>
</ErrorBoundary>
</ChangelogButton>
</div>
</Col>
</Row>

<MainContainer fluid>
<MainRow className={'gx-0'}>
<MainCol>{children}</MainCol>
</MainRow>
</MainContainer>

<FooterRow className={'gx-0'}>
<FooterCol>
<FooterContent />
</FooterCol>
</FooterRow>

<Snowfall />
<Santa />
</Container>
)
}
124 changes: 82 additions & 42 deletions web/src/components/Layout/NavigationBar.tsx
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only nitpicking. It would be nice to distinguish the levels of concern here. So the basic structure of the header is

<Brand />
<NavigationLinks />
<CallToAction />

And then have each of them in a separate component. This would make it easier fo follow I'm not sure if this works with the bootstrap way.

Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { useRouter } from 'next/router'
import React, { useCallback, useMemo, useState } from 'react'
import React, { Suspense, useCallback, useMemo, useState } from 'react'

import { styled } from 'styled-components'
import {
Expand All @@ -15,6 +15,9 @@ import {
import classNames from 'classnames'
import { FaGithub, FaTwitter } from 'react-icons/fa'

import Image from 'next/image'
import { ErrorBoundary } from 'react-error-boundary'
import { NomenclatureSwitch } from '../Common/NomenclatureSwitch'
import BrandLogoBase from 'src/assets/images/logo.svg'
import BrandLogoLargeBase from 'src/assets/images/logo_text_right.svg'

Expand All @@ -24,6 +27,10 @@ import { LinkExternal } from 'src/components/Link/LinkExternal'
import { TWITTER_USERNAME_RAW, URL_GITHUB } from 'src/constants'
import { ChristmasToggle } from 'src/components/Common/Christmas'
import { LanguageSwitcher } from 'src/components/Layout/LanguageSwitcher'
import GisaidLogoPNG from 'src/assets/images/GISAID_logo.png'
import { ChangelogButton } from 'src/components/Common/ChangelogButton'
import { FetchError } from 'src/components/Error/FetchError'
import { LastUpdated } from 'src/components/Common/LastUpdated'

export function matchingUrl(url: string, pathname: string): boolean {
if (pathname.startsWith('/variants')) {
Expand Down Expand Up @@ -164,48 +171,81 @@ export function NavigationBar() {
}, [t])

return (
<Navbar expand="md" color="light" light role="navigation">
<Link href="/">
<BrandLogoLarge className="d-none d-lg-block" />
<BrandLogo className="d-block d-lg-none" />
</Link>

<NavbarToggler onClick={toggle} />

<Collapse isOpen={isOpen} navbar>
<NavWrappable navbar>
{Object.entries(navLinksLeft).map(([url, text]) => {
return (
<NavItem key={url} className={classNames(matchingUrl(url, pathname) && 'active')}>
<NavLink tag={Link} href={url}>
{text}
<>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just saw this: Maybe to add the <header> is also this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

When I am on a small screen, I see that the the github logo gets the tag: 'Fork'. Why is this the case. It seems wrong.
grafik

<Navbar expand="md" color="light" light role="navigation">
<Link href="/">
<BrandLogoLarge className="d-none d-lg-block" />
<BrandLogo className="d-block d-lg-none" />
</Link>

<NavbarToggler onClick={toggle} />

<Collapse isOpen={isOpen} navbar>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the way to have the hamburger menu in bootstrap? I would have assumed, that one has a construct like this:

<div className='d-none d-md-flex'>
  <HamburgerMenu />
</div>
<div> className='flex d-md-none'>
  <RegularMenu>
</div>

It would then be easier to also add the SubNavigationBar with two different variants, one for the RegularMenu and one for the HamburgerMenu.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This would also make it clearer, why you have the different stylings for the SubNavigationBar.

<NavWrappable navbar>
{Object.entries(navLinksLeft).map(([url, text]) => {
return (
<NavItem key={url} className={classNames(matchingUrl(url, pathname) && 'active')}>
<NavLink tag={Link} href={url}>
{text}
</NavLink>
</NavItem>
)
})}
</NavWrappable>

<Nav className="ms-auto" navbar>
<NavItem>
<Row className={'gx-0'}>
<Col className="mt-2 mx-3">
<ChristmasToggle />
</Col>
</Row>
</NavItem>
{navLinksRight.map(({ text, title, url, alt, icon }) => (
<NavItem key={title}>
<NavLink tag={LinkRight} title={title} href={url} alt={alt} icon={null}>
<span>
<span className="me-2">{icon}</span>
<span className="d-inline d-sm-none">{text}</span>
</span>
</NavLink>
</NavItem>
)
})}
</NavWrappable>

<Nav className="ms-auto" navbar>
<NavItem>
<Row className={'gx-0'}>
<Col className="mt-2 mx-3">
<ChristmasToggle />
</Col>
</Row>
</NavItem>
{navLinksRight.map(({ text, title, url, alt, icon }) => (
<NavItem key={title}>
<NavLink tag={LinkRight} title={title} href={url} alt={alt} icon={null}>
<span>
<span className="me-2">{icon}</span>
<span className="d-inline d-sm-none">{text}</span>
</span>
</NavLink>
</NavItem>
))}
<LanguageSwitcher />
</Nav>
</Collapse>
</Navbar>
))}
<LanguageSwitcher className="mx-auto mx-md-0" />
</Nav>
</Collapse>
</Navbar>

{!isOpen && <SubNavigationBar className="d-none d-md-flex mt-2 gx-0 px-3" />}
{isOpen && <SubNavigationBar className="d-flex mt-2 gx-0 px-3" />}
Comment on lines +219 to +220
Copy link
Collaborator

Choose a reason for hiding this comment

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

{ isOpen? <SubNavigationBar className="d-flex mt-2 gx-0 px-3" /> : <SubNavigationBar className="d-none d-md-flex mt-2 gx-0 px-3" /> }

Does this do the same? This would convey the meaning, that either one or the other is shown.

</>
)
}

function SubNavigationBar({ className }: { className?: string }) {
const { t } = useTranslationSafe()
return (
<div className={className}>
<GisaidText className="d-none d-md-flex align-items-center gap-1">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it intentional, that you dont show the GisaidText on small screens?

{t('Enabled by data from {{ gisaid }}', { gisaid: '' })}
<LinkExternal href="https://www.gisaid.org/" icon={null}>
<Image src={GisaidLogoPNG} alt="GISAID" height={27} width={73} />
</LinkExternal>
</GisaidText>

<NomenclatureSwitch className="mx-auto mx-md-0 ms-md-auto" />

<ChangelogButton className="d-none d-md-flex">
<ErrorBoundary FallbackComponent={FetchError}>
<Suspense>
<LastUpdated />
</Suspense>
</ErrorBoundary>
</ChangelogButton>
</div>
)
}

const GisaidText = styled.small`
font-size: 0.9rem;
`
Loading