506 update shared mutations page #529
Conversation
AdvancedCodingMonkey
temporarily deployed
to
refs/heads/staging
GitHub Actions
Inactive
AdvancedCodingMonkey
force-pushed
the
506-update-shared-mutations-page
branch
from
fb34877 to
dae4890
Compare
AdvancedCodingMonkey
temporarily deployed
to
refs/heads/staging
GitHub Actions
Inactive
AdvancedCodingMonkey
requested review from
fengelniederhammer and
JonasKellerer
AdvancedCodingMonkey
temporarily deployed
to
refs/heads/staging
GitHub Actions
Inactive
There was a problem hiding this comment.
Looks good to me and works flawless.
I'm not sure if it is this PR, but it would be nice to do some refactoring of the navbar. But I will not block merging for this.
We also discussed this on the call, but I think it would be nice to use more of the bootstrap features, so we don't have to build our own custom solutions. But this is only my preferance.
| <span className="ms-1" title={full}> | ||
| {t('Last updated: {{ date }}', { date: date })} | ||
| </span> | ||
| <span title={full}>{t('Last updated: {{ date }}', { date: date })}</span> |
There was a problem hiding this comment.
| <span title={full}>{t('Last updated: {{ date }}', { date: date })}</span> | |
| <span title={full}>{t('Last updated: {{ date }}', { date })}</span> |
Does this work als well?
| @@ -72,15 +73,13 @@ export function ToggleTwoLabels({ | |||
| return ( | |||
| <Label htmlFor={identifier} className={className} title={title}> | |||
There was a problem hiding this comment.
Is the htmlFor necessary? I thought this should already work, since the input element is inside the label?
| <ToggleTwoLabelsBase | ||
| id={identifier} | ||
| className="react-toggle-two-labels-custom" | ||
| icons={false} | ||
| onChange={onChange} | ||
| {...props} | ||
| /> |
There was a problem hiding this comment.
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>
| @@ -95,7 +104,7 @@ export function SharedMutationsTable() { | |||
| {t('Shared mutations')} | |||
| <AdvancedToggleWrapper> | |||
| {t('Sort by: ')} | |||
| <ToggleTwoLabels | |||
| <PositionToggle | |||
There was a problem hiding this comment.
I think a plain radio would work here as well. This would be extensible, and we would not have the trouble with the styling.
| @@ -84,7 +93,7 @@ export function SharedMutationsTable() { | |||
| <TableHeader> | |||
| <Tr> | |||
| {variants.map((variant) => ( | |||
| <Th key={variant}>{variant}</Th> | |||
| <Th key={variant}>{enablePangolin ? variant.split('\n')[1].trim() : variant.split('\n')[0].trim()}</Th> | |||
There was a problem hiding this comment.
This is some very special logic for the variant string. Maybe not this PR, but can't we have this in some form of type?
interface Variant {
title: string
}
interface Clade {
title: string
subtitle: string
}
(I'm not sure about the naming of the interfaces and the title/subsitle, but just to give a hint what I mean).
If this is not possible, it would be nice to have this in a function if we want to change this later on.
| <NavItem key={url} className={classNames(matchingUrl(url, pathname) && 'active')}> | ||
| <NavLink tag={Link} href={url}> | ||
| {text} | ||
| <> |
There was a problem hiding this comment.
I just saw this: Maybe to add the <header> is also this PR.
There was a problem hiding this comment.
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.
| {!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" />} |
There was a problem hiding this comment.
{ 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.
There was a problem hiding this comment.
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.
|
|
||
| <NavbarToggler onClick={toggle} /> | ||
|
|
||
| <Collapse isOpen={isOpen} navbar> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This would also make it clearer, why you have the different stylings for the SubNavigationBar.
| const { t } = useTranslationSafe() | ||
| return ( | ||
| <div className={className}> | ||
| <GisaidText className="d-none d-md-flex align-items-center gap-1"> |
There was a problem hiding this comment.
Is it intentional, that you dont show the GisaidText on small screens?
Resolves #506
This PR makes content of the shared mutations page responsive to the nomenclature toggle:
Additionally, the layout of the navigation bar was adjusted, such that items are properly aligned when the collapsed navigation bar is opened on a small screen.
Screencast.from.25.02.2025.09.39.50.webm
Screencast.from.25.02.2025.09.40.21.webm