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.

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

Various fixes #314

Merged
merged 10 commits into from
Nov 14, 2023
Merged

Various fixes #314

merged 10 commits into from
Nov 14, 2023

Conversation

chrisbendel
Copy link
Collaborator

@chrisbendel chrisbendel commented Nov 8, 2023

Addresses multiple points on this ticket

return (
<>
<Global
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved to main.scss

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I slightly prefer it here because this way we only hide the osano cookie link if we're also rendering an alternative method. If the hiding is in the sass, it's possible that we hide it, but then don't render this on some screens

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should have added another comment for context woops, we moved the manage cookies link into the user name/dropdown thing at the top right, with that being there it's not technically rendered until the dropdown is opened. This way every user should have access to it (ie before the researcher experience doesn't have a footer so the cookie was still rendering)

@chrisbendel chrisbendel marked this pull request as ready for review November 13, 2023 16:57
Copy link
Member

@nathanstitt nathanstitt left a comment

Choose a reason for hiding this comment

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

looks good just a few accessibility things and the link

/>
<a href='#' onClick={showOsano}>Manage Cookies</a>
</>
<span onClick={showOsano}>Manage Cookies</span>
Copy link
Member

Choose a reason for hiding this comment

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

if you want to use a span, this needs to have cursor:hover and also a role="button" for a18y

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nathanstitt the cursor portion is taken care of by the parent link

<StyledLink to='#'>
    <Menu.Item>
        <ManageCookiesLink />
    </Menu.Item>
</StyledLink>

the result:

Screenshot 2023-11-13 at 3 23 45 PM

Question is, would we still want the role on the span? or would this parent cover that case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nathanstitt the cursor portion is taken care of by the parent link

<StyledLink to='#'>
    <Menu.Item>
        <ManageCookiesLink />
    </Menu.Item>
</StyledLink>

the result:

Screenshot 2023-11-13 at 3 23 45 PM

Question is, would we still want the role on the span? or would this parent cover that case?

Copy link
Member

Choose a reason for hiding this comment

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

nah, sorry I'd left that comment before I realized it was being used in the menu

<Box align='center' gap='small' onClick={() => step.backAction?.()}>
<Icon icon='chevronLeft'></Icon>
<span>Back</span>
</Box>
</Link> : <span></span>}
</FakeLink> : <span></span>}
Copy link
Member

Choose a reason for hiding this comment

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

should add role="link" to the span for screenreader

return (
<>
<Global
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I slightly prefer it here because this way we only hide the osano cookie link if we're also rendering an alternative method. If the hiding is in the sass, it's possible that we hide it, but then don't render this on some screens

@chrisbendel chrisbendel merged commit 0d39668 into main Nov 14, 2023
4 checks passed
@chrisbendel chrisbendel deleted the post-prod-fixes branch November 14, 2023 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants