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

EQM: Side panel back / close icon UX improvements #12311

Conversation

nucleogenesis
Copy link
Member

Summary

  • Removes using onClickOutside, reworks logic around "canGoBack"
  • Moves the X icon into the "heading" slot in the side panel and sets it up so that it will match other things put into the heading the same way
  • Applies changes listed in EQM: Side panels navigation handling improvements #12308

References

Closes #12308

Reviewer guidance

Please check for regressions wherever else the side panel is used in the Learn library area.

Also, see that the UX changes defined in the linked issue are applied and that the UX is improved overall while using the side panel throughout quiz creation.

@nucleogenesis nucleogenesis added the TODO: needs review Waiting for review label Jun 18, 2024
@github-actions github-actions bot added APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend SIZE: medium labels Jun 18, 2024
Comment on lines +30 to +37
<KIconButton
v-if="closeButtonIconType"
:icon="closeButtonIconType"
class="close-button"
:style="closeButtonStyle"
:ariaLabel="closeButtonMessage"
:tooltip="closeButtonMessage"
@click="closePanel"
/>
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved this into here so that the rest of the stuff in .header-content is in the same container for spacing for the icon buttons.

@@ -20,27 +20,25 @@

<!-- Fixed header -->
<div
v-show="$slots.header"
Copy link
Member Author

Choose a reason for hiding this comment

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

Not needed now since the icon button always lives here

@@ -5,10 +5,18 @@
ref="resourcePanel"
alignment="right"
sidePanelWidth="700px"
:closeButtonIconType="closeBackIcon"
closeButtonIconType="close"
Copy link
Member Author

Choose a reason for hiding this comment

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

We're injecting the "back" icon where/when we want it now into the #header slot, so we hard-code "close" here so that the X is always there.

@nucleogenesis nucleogenesis requested review from pcenov June 24, 2024 16:16
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

One thing we should change.

}
router.push({
name: PageNames.EXAM_CREATION_ROOT,
params: { ...route.value.params },
Copy link
Member

Choose a reason for hiding this comment

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

Should be specific here, see e.g.

(although I noticed in searching for this, there are still lots of places just doing a general spread).

Because of the way Vue router works, items that are not in the URL still end up in the params state, which can cause unexpected bugs down the line.

@rtibbles rtibbles dismissed their stale review June 25, 2024 03:31

Issue addressed, waiting for manual QA for approval.

@pcenov
Copy link
Member

pcenov commented Jun 25, 2024

Hi @nucleogenesis - could you please have a look why some checks haven't completed yet here so that I can test the latest build?

@nucleogenesis nucleogenesis force-pushed the fix--sidepanel-routing-icon branch from e66ebac to b68a309 Compare June 25, 2024 21:48
@nucleogenesis
Copy link
Member Author

@pcenov I fixed the merge conflict and seems to have gotten things flowing again on the checks

Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

LGTM - Implemented as specified by Jacob, no issues observed while regression testing the side panel.

@nucleogenesis nucleogenesis merged commit a06464b into learningequality:develop Jun 26, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend SIZE: medium TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EQM: Side panels navigation handling improvements
3 participants