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

Make short mission info non-clickable and add button #535 #841

Merged
merged 11 commits into from
Mar 4, 2024
+48 −12
Merged

Make short mission info non-clickable and add button #535 #841

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ddaff61
changed button to themed button and aria-hid the info button
AAloshine-scottlogic Feb 23, 2024
d736a10
lint importing fix
AAloshine-scottlogic Feb 23, 2024
1fbcb6f
fixed testing
AAloshine-scottlogic Feb 23, 2024
3c7e723
got rid of extra padding by getting rid of invisible border
AAloshine-scottlogic Feb 26, 2024
a469f31
pr request fixes (css)
AAloshine-scottlogic Feb 27, 2024
91bc428
fixed testing and formatting
AAloshine-scottlogic Mar 1, 2024
9720fdb
test now works!
AAloshine-scottlogic Mar 1, 2024
f414411
lint
AAloshine-scottlogic Mar 1, 2024
f7d5770
added defaultnormalising before text match on testing
AAloshine-scottlogic Mar 1, 2024
d6eb13a
got rid of console.log
AAloshine-scottlogic Mar 1, 2024
29016f8
rid of min width
AAloshine-scottlogic Mar 1, 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
26 changes: 22 additions & 4 deletions frontend/src/components/LevelMissionInfoBanner/LevelMissionInfoBanner.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
.level-mission-info-banner {
display: flex;
gap: 0.5rem;
gap: 1.5rem;
align-items: center;
padding: 0.5rem 1.5rem;
border: 0.5rem solid transparent;
background-color: var(--level-banner-background-colour);
color: var(--level-banner-text-colour);
text-align: left;
Expand All @@ -17,15 +16,34 @@

.level-title-area {
min-width: 10%;
Copy link
Member

Choose a reason for hiding this comment

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

Ah now I remember. I wanted to point out that this min-width doesn't look nice on a wide screen:

image

Better to use nowrap instead, then bump up the gap on the parent a little:

.level-mission-info-banner {
  gap: 1.5rem;
}
.level-title-area {
  white-space: nowrap;
}

image

Could you fix that for us please? Thanks! Note that 1.5rem was just a guess, so if you think it looks better with a touch more gap, then feel free to change that.

margin: 1rem 0;
Copy link
Member

Choose a reason for hiding this comment

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

Need to remove min-width: 10% above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah sorry misssed that bit

font-weight: 800;
font-size: 1.875rem;
white-space: nowrap;
}

.level-mission-info-banner > p {
margin: 0;
font-size: 1rem;
}

.level-mission-info-banner:hover {
background-color: var(--level-banner-background-colour-hover);
.level-mission-info-banner .themed-button {
gap: 0.5rem;
height: auto;
padding: 0.75rem 1rem;
white-space: nowrap;
AAloshine-scottlogic marked this conversation as resolved.
Show resolved Hide resolved
}

.info-icon {
display: flex;
align-items: center;
box-sizing: border-box;
height: auto;
padding: 0 0.625rem;
border-radius: 50%;
background-color: var(--action-button-text-colour);
color: var(--action-button-background-colour);
font: 1rem serif;
font-weight: bold;
aspect-ratio: 1;
}
20 changes: 16 additions & 4 deletions frontend/src/components/LevelMissionInfoBanner/LevelMissionInfoBanner.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import { render, screen, fireEvent } from '@testing-library/react';
import {
render,
screen,
fireEvent,
getDefaultNormalizer,
} from '@testing-library/react';
import { describe, expect, test, vi } from 'vitest';

import { LEVELS } from '@src/Levels';
Expand All @@ -16,9 +21,16 @@ describe('LevelMissionInfoBanner component tests', () => {
/>
);

const button = screen.getByRole('button');
const expectedContent = LEVELS[currentLevel].missionInfoShort ?? '';
expect(button).toContainHTML(expectedContent);
const expectedContent = LEVELS[currentLevel].missionInfoShort;

if (!expectedContent)
throw new Error(`No missionInfoShort found for level ${currentLevel}`);

const expectedText = getDefaultNormalizer()(
expectedContent.slice(0, expectedContent.indexOf(' <u>'))
);
const banner = screen.getByText(expectedText);
expect(banner).toContainHTML(expectedContent);
});

test('fires the openOverlay callback on button click', () => {
Expand Down
13 changes: 10 additions & 3 deletions frontend/src/components/LevelMissionInfoBanner/LevelMissionInfoBanner.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { LEVELS } from '@src/Levels';
import ThemedButton from '@src/components/ThemedButtons/ThemedButton';
import { LEVEL_NAMES } from '@src/models/level';

import './LevelMissionInfoBanner.css';
Expand All @@ -11,14 +12,20 @@ function LevelMissionInfoBanner({
openOverlay: () => void;
}) {
return (
<button className="level-mission-info-banner" onClick={openOverlay}>
<span className="level-title-area">{`Level ${currentLevel + 1}`}</span>
<span className="level-mission-info-banner">
Copy link
Member

Choose a reason for hiding this comment

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

@dhinrichs-scottlogic Would we be better off using some sort of landmark for this banner, or is it self-explanatory?

I'm wondering if "section" or "complementary" are appropriate. I do realise <section> is a bit controversial...
https://developer.mozilla.org/en-US/blog/aria-accessibility-html-landmark-roles/

Copy link
Contributor

Choose a reason for hiding this comment

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

any semantic element is better than a <span>.
If I follow this flowchart, <article> or <section> seems reasonable

Copy link
Contributor

Choose a reason for hiding this comment

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

landmarks should be used sparingly. If we give this a section, with a header, a text paragraph and a button, I think there's enough context for it without giving it a landmark role, and you already can easily jump to its header. The first rule of aria is: don't use aria if you can avoid it, that's probably true for role declarations, too.
mdn's comedians are putting it aptly here:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

I vote for <article>

Copy link
Member

Choose a reason for hiding this comment

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

Well, that's a possibility. Another is that they are list items within a list inside the chat panel, which itself has role=log:
https://www.w3.org/WAI/WCAG21/Techniques/aria/ARIA23
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/log_role

The first link has exactly our use case, plus a working demo. I note once again that Narrator is not announcing "polite" additions to the conversation log, but NVDA is.

Do we want to add this improvement as a separate issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that's our use case, the short mission info doesn't keep a log, or updates, it just contains different text between the levels, which is expected since we are changing context. The same is true for the handbook, control panel, level buttons...
I'd say keep it as simple as possible, use either <section> or <article>, it's at least a little bit more semantic than <span>, but don't add any additional roles or aria for now.

<h2 className="level-title-area">{`Level ${currentLevel + 1}`}</h2>
<p
dangerouslySetInnerHTML={{
__html: LEVELS[currentLevel].missionInfoShort ?? '',
}}
></p>
</button>
<ThemedButton onClick={openOverlay}>
<span className="info-icon" aria-hidden="true">
i
</span>
Mission Info
</ThemedButton>
</span>
);
}
export default LevelMissionInfoBanner;
Loading