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

Jump to bottom

334 move info message to handbook #345

Merged
merged 11 commits into from
Oct 12, 2023
Merged

334 move info message to handbook #345

merged 11 commits into from
Oct 12, 2023

Conversation

heatherlogan-scottlogic
Copy link
Contributor

@heatherlogan-scottlogic heatherlogan-scottlogic commented Oct 4, 2023 •
edited
Loading

changes

  • separated the overlay and handbook overlay components (generic overlay could cover welcome/pop up info messages etc while handbook would be it's own thing)
  • when user changes level, the mission info overlay pops up
  • added tabs to handbook to switch between pages
  • added mission info to pages

Qs

  • for now the mission info pops up every time the user switches phase regardless of if they have seen it before. This shouldn;t be a problem if we get rid of the level buttons and transition level automatically. But we might want to change it to only showing when the user enters a level for the first time?

@heatherlogan-scottlogic heatherlogan-scottlogic linked an issue Oct 4, 2023 that may be closed by this pull request
move info message to overlay #334
Closed
@heatherlogan-scottlogic heatherlogan-scottlogic changed the base branch from main to dev October 4, 2023 14:23
@heatherlogan-scottlogic heatherlogan-scottlogic force-pushed the 334-move-info-message-to-handbook branch from b4d3b3e to dd2d373 Compare October 4, 2023 15:27
@heatherlogan-scottlogic heatherlogan-scottlogic marked this pull request as ready for review October 4, 2023 16:27
gsproston-scottlogic
gsproston-scottlogic suggested changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@gsproston-scottlogic gsproston-scottlogic left a comment

Choose a reason for hiding this comment

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

I'm a big fan of this! Just a small naming thing from me.

I'm also seeing a little visual bug for when the handbook has tabs, the top left corner shouldn't be rounded if the tabs are present:
image

frontend/src/components/HandbookOverlay/HandbookOverlayTabs.css Outdated
@@ -0,0 +1,57 @@
.tabs {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be best to make this a more specific name, maybe handook-tabs or something, just incase we end up using tabs anywhere else in the app.

@heatherlogan-scottlogic
Copy link
Contributor Author

heatherlogan-scottlogic commented Oct 5, 2023 •
edited
Loading

I'm a big fan of this! Just a small naming thing from me.

I'm also seeing a little visual bug for when the handbook has tabs, the top left corner shouldn't be rounded if the tabs are present: image

ah missed that! Working on now

  • moved the tabs in a bit instead - thinking it looks more like a folder this way?
    image

@gsproston-scottlogic
Copy link
Contributor

I'm a big fan of this! Just a small naming thing from me.
I'm also seeing a little visual bug for when the handbook has tabs, the top left corner shouldn't be rounded if the tabs are present: image

ah missed that! Working on now

  • moved the tabs in a bit instead - thinking it looks more like a folder this way?
    image

Ah yes I much prefer that!

I've also just noticed, we're showing a 'attacks' tab in the handbook on level 1. Do we want to hide that just for level 1?

gsproston-scottlogic
gsproston-scottlogic approved these changes Oct 6, 2023
View reviewed changes
Copy link
Contributor

@gsproston-scottlogic gsproston-scottlogic left a comment

Choose a reason for hiding this comment

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

Love it 👍

chriswilty
chriswilty requested changes Oct 10, 2023
View reviewed changes
Copy link
Member

@chriswilty chriswilty left a comment

Choose a reason for hiding this comment

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

👍 Very nice! Just a few comments.

frontend/src/components/HandbookOverlay/HandbookOverlay.css Show resolved Hide resolved
frontend/src/components/HandbookOverlay/HandbookOverlayTabs.tsx Show resolved Hide resolved
frontend/src/components/Overlay/MissionInformation.css Show resolved Hide resolved
frontend/src/components/HandbookOverlay/HandbookOverlayTabs.css Show resolved Hide resolved
frontend/src/components/Overlay/OverlayWelcome.css Show resolved Hide resolved
frontend/src/components/Overlay/OverlayWelcome.tsx Show resolved Hide resolved
@chriswilty
Copy link
Member

As suggested on Teams, I could make the suggested id changes and check accessibility in #319, then you could just merge this one on monday.

@chriswilty chriswilty mentioned this pull request Oct 12, 2023
Merged
@chriswilty
Copy link
Member

Changes made in #391, approving this one 🎉

@chriswilty chriswilty self-requested a review October 12, 2023 14:12
@chriswilty chriswilty merged commit 98fc75b into dev Oct 12, 2023
2 checks passed
@chriswilty chriswilty deleted the 334-move-info-message-to-handbook branch October 12, 2023 14:14
chriswilty pushed a commit that referenced this pull request Apr 8, 2024
@heatherlogan-scottlogic @chriswilty
334 move info message to handbook (#345)
a6f7fb7 
This work unifies the overlay for the three main popups: welcome message, mission info, handbook.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gsproston-scottlogic gsproston-scottlogic gsproston-scottlogic approved these changes

@chriswilty chriswilty chriswilty approved these changes

@pmarsh-scottlogic pmarsh-scottlogic Awaiting requested review from pmarsh-scottlogic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

move info message to overlay
3 participants
@heatherlogan-scottlogic @gsproston-scottlogic @chriswilty