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

[Accordion] Add "collapse all"/ "expand all" logic into AccordionContainer #11565

Conversation

AlexVelezLl
Copy link
Member

@AlexVelezLl AlexVelezLl commented Nov 29, 2023

Summary

  • Refactor accordion header logic to be into the "AccordionContainer" component
  • Improve AccordionContainer styles to match closer Figma specs

Before
image

After
image

References

Closes #11547

Reviewer guidance

  1. Enable quizCreation debug mode by replacing const quizForge = useQuizCreation(true); here
  2. Check quizCreation keeps working the same as before this PR.

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

@AlexVelezLl AlexVelezLl added TODO: needs review Waiting for review APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend labels Nov 29, 2023
@AlexVelezLl AlexVelezLl changed the base branch from release-v0.16.x to develop November 29, 2023 13:56
@nucleogenesis
Copy link
Member

Thanks @AlexVelezLl !

This will need to be rebased once #11434 is merged and you might run into some minor conflicts I'll be happy to help resolve.

@@ -185,57 +185,34 @@
</KGrid>

<AccordionContainer
:items="quizForge.activeQuestions.value"
@toggled="items => expandedQuestionIds = items"
:items="quizForge.activeQuestions.value.map(i => ({
Copy link
Member Author

Choose a reason for hiding this comment

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

I would suggest that we can make items a generic array of objects that contains, for example, the id attribute (instead of something specific like "question_id"), and that from this we have the criteria to know which items to choose expand when the "expand all" button is clicked.

Copy link
Member

Choose a reason for hiding this comment

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

Excellent idea!

};
},
watch: {
expandedItemIds() {
this.$emit('toggled', this.expandedItemIds);
Copy link
Member Author

Choose a reason for hiding this comment

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

By handling the collapse all and expand all within the AccordionComponent, I think we can handle the functionality of the expandedItemIds within the component without having to expose the entire array, and just expose the isItemExpanded method.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed 👍🏻

@AlexVelezLl AlexVelezLl changed the title [Accordion] Add "collapse all" logic into AccordionContainer [Accordion] Add "collapse all"/ "expand all" logic into AccordionContainer Dec 4, 2023
Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Great work @AlexVelezLl thank you!

I tested it manually and it works as expected. Thanks for the additional clean-up and normalizing the component to use a generic id property on the object passed in for the items that's excellent <3

@@ -185,57 +185,34 @@
</KGrid>

<AccordionContainer
:items="quizForge.activeQuestions.value"
@toggled="items => expandedQuestionIds = items"
:items="quizForge.activeQuestions.value.map(i => ({
Copy link
Member

Choose a reason for hiding this comment

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

Excellent idea!

};
},
watch: {
expandedItemIds() {
this.$emit('toggled', this.expandedItemIds);
Copy link
Member

Choose a reason for hiding this comment

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

Agreed 👍🏻

type: Array,
required: true,
function(value) {
return value.every(item => typeof item === 'object' && 'id' in item);
Copy link
Member

Choose a reason for hiding this comment

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

TIL 🤯 'id' in { id: 1 } => true

So many times I've written Object.keys(theObj).includes(theKey) and I could have just used in

@AlexVelezLl AlexVelezLl merged commit 3203922 into learningequality:develop Dec 5, 2023
34 checks passed
@AlexVelezLl AlexVelezLl deleted the accordioncontainer-code-layout branch December 5, 2023 12:55
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.) SIZE: medium SIZE: small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "expand all" and "collapse all" functionality into the AccordionContainer component
2 participants