8000 Fix CCJ-123: add "close" button to victory modal by nwinter · Pull Request #7764 · codecombat/codecombat · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix CCJ-123: add "close" button to victory modal #7764

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

Merged
merged 1 commit into from
Oct 16, 2024
Merged

Fix CCJ-123: add "close" button to victory modal #7764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions app/styles/play/level/modal/course-victory-modal.sass
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
.modal-content
position: relative
margin-top: -251px
box-shadow: none

//- Header

Expand Down
6 changes: 6 additions & 0 deletions app/styles/play/level/modal/hero-victory-modal.sass
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,12 @@
min-height: 30px
margin-top: 160px

.close-button
position: absolute
right: -15px
top: -37px
font-size: 50px

.achievement-panel
background: transparent url("/images/pages/play/level/modal/victory_modal_shelf.png") no-repeat center 73px
width: 824px
Expand Down
10000 2 changes: 2 additions & 0 deletions app/templates/play/level/modal/hero-victory-modal.pug
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ block modal-header-content
h1(data-i18n="play_level.victory") Victory

block modal-body-content
button.close-button.btn.btn-illustrated.btn-lg.btn-warning(type="button", data-dismiss="modal", aria-hidden="true", tabindex=-1) ×

Comment on lines +13 to +14
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider relocating the close button to the modal header

The close button has been added to the modal body content, which is an unusual placement. Typically, close buttons are located in the modal header for consistency and user expectation.

  1. Consider moving this button to the modal-header-content block for better alignment with standard modal designs.
  2. The tabindex="-1" attribute may interfere with keyboard navigation. Consider removing it to allow natural tab order, or ensure it doesn't negatively impact accessibility.
  3. For better accessibility, consider adding an aria-label to the button, e.g., aria-label="Close".

Here's a suggested relocation of the close button to the modal header:

block modal-header-content
  button.close-button.btn.btn-illustrated.btn-lg.btn-warning(type="button", data-dismiss="modal", aria-hidden="true", aria-label="Close") ×
  #victory-header.out
    // ... rest of the header content

This change would place the close button in a more standard location and improve its accessibility.

if victoryText
#victory-text(dir="auto")= victoryText

Expand Down
12 changes: 7 additions & 5 deletions app/views/core/ModalView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ module.exports = (ModalView = (function () {
render () {
__guardMethod__(this.focusTrap, 'deactivate', o => o.deactivate())
super.render()
return this.trapFocus()
this.trapFocus()
}

afterRender () {
Expand Down Expand Up @@ -83,12 +83,14 @@ module.exports = (ModalView = (function () {

trapFocus () {
if (!this.trapsFocus) { return }
console.log(this.constructor != null ? this.constructor.name : undefined, 'trapping focus within modal')
if (this.focusTrap == null) { this.focusTrap = focusTrap.createFocusTrap(this.el) }
if (!this.focusTrap) {
this.focusTrap = focusTrap.createFocusTrap(this.el)
}
try {
return (this.focusTrap != null ? this.focusTrap.activate() : undefined)
this.focusTrap?.activate()
console.log(this.constructor?.name, 'trapping focus within modal')
} catch (e) {
return console.log(this.constructor != null ? this.constructor.name : undefined, 'not trapping focus for modal with no focusable elements')
console.log(this.constructor?.name, 'not trapping focus for modal with no focusable elements')
}
}

Expand Down
1 change: 1 addition & 0 deletions app/views/play/level/modal/HeroVictoryModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,7 @@ module.exports = (HeroVictoryModal = (function () {
for (const original in this.thangTypes) { const hero = this.thangTypes[original]; this.playSelectionSound(hero, true) } // Preload them
this.updateSavingProgressStatus()
this.initializeAnimations()
_.delay(() => this.$el?.find('button.btn-success').last()?.focus(), 200)
if (this.level.isLadder()) {
this.ladderSubmissionView = new LadderSubmissionView({ session: this.session, level: this.level })
return this.insertSubView(this.ladderSubmissionView, this.$el.find('.ladder-submission-view'))
Expand Down
Loading
0