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

Add learning activity bar component #8151

Merged
merged 2 commits into from
Jun 21, 2021

Conversation

MisRob
Copy link
Member

@MisRob MisRob commented Jun 14, 2021

Summary

Add a new, mobile responsive, learning activity bar component.

Note that connecting the bar to the app is out of the scope of this ticket and that if you follow the testing instructions below it will appear sticky even when it's not supposed to be. It's not related to the new component itself, but rather to CoreBase and will need to be resolved when we start connecting new bars to pages. Also, since this PR only adds a component to our library, I didn't update any Gherkin stories at this point.

Screenshots

Only the most important states are documented here. Please see Figma designs linked in references to compare to the full specification.

Bar/menu items distribution

320-480 480-600 600 and higher
320-480 480-600 600-higher

Bookmarks

Bookmarked resource Not bookmarked resource
Bar bookmarked-bar not-bookmarked-bar
Menu bookmarked-menu not-bookmarked-menu

Context

Lesson context Non-lesson context
Bar lesson-context-bar non-lesson-context-bar
Menu lesson-context-menu non-lesson-context-menu

When a resource can't be manually marked as complete

Bar Menu
cannot-be-marked-complete-bar cannot-be-marked-complete-menu

References

Reviewer guidance

I haven't found a better way to test the new bar than hacking CoreBase locally as demonstrated here and then navigating to a resource page, for example, "Learn" -> "Channels" -> Select a channel and open a resource from a topic of the channel.

  • Make sure to run yarn install --check-files to pull the latest KDS version containing learning activities icons.
  • Check for different resolutions
  • Check for RTL language (you can replace /en/ by /ar/ in the URL)
  • Check accessibility - navigation in the bar, opening the menu, navigation in the menu

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@MisRob MisRob force-pushed the learning-activity-bar branch 2 times, most recently from 4d4e454 to 7a2e58d Compare June 15, 2021 12:29
@MisRob MisRob force-pushed the learning-activity-bar branch 8 times, most recently from 260c47d to 7526493 Compare June 17, 2021 09:56
@MisRob MisRob force-pushed the learning-activity-bar branch from 7526493 to 3651e77 Compare June 17, 2021 11:04
@MisRob MisRob added the TODO: needs review Waiting for review label Jun 17, 2021
@MisRob MisRob changed the title WIP Add learning activity bar component Add learning activity bar component Jun 17, 2021
@MisRob MisRob marked this pull request as ready for review June 17, 2021 11:59
</template>
<TextTruncator
:text="resourceTitle"
:maxHeight="26"
Copy link
Member Author

Choose a reason for hiding this comment

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

This magical number seems to work though I am not sure how exactly. Let me know if you see some possible issues with it or have more insight into if this is an appropriate value.

</KLabeledIcon>

<template #icon>
<KIconButton
Copy link
Member Author

@MisRob MisRob Jun 17, 2021

Choose a reason for hiding this comment

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

I think it may be a good idea to add ariaLabel and tooltip for the back navigation button but am not sure yet and don't know what the string would look like. See https://learningequality.slack.com/archives/C023DNC5GCX/p1623850266060400. EDIT: Done

Copy link
Member Author

@MisRob MisRob Jun 17, 2021

Choose a reason for hiding this comment

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

Also, it may need to be wrapped in router-link in the future depending on how we'll use the component. I took the simplest way for now thinking that we can update it together with a new prop for the link destination, instead of emitting navigateBack event if needed. The same applies to some other buttons in the bar that may have some connection to the router.

@marcellamaki marcellamaki self-requested a review June 21, 2021 13:24
Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

This looks great @MisRob! Manual QA looks good to me, and the code is so easy to read. I love the comments, too. Nice work ✨ 🙂

@marcellamaki marcellamaki merged commit e0606d1 into learningequality:develop Jun 21, 2021
@MisRob MisRob deleted the learning-activity-bar branch July 22, 2021 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create new base top bar component with mobile responsive breakpoints
2 participants