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

Added another condition for TOPICS_TOPIC_SEARCH redirection #12019

Open
wants to merge 3 commits into
base: release-v0.16.x
Choose a base branch
from

Conversation

Wck-iipi
Copy link
Contributor

Summary

User can now see content instead of search bar. Also, I changed the not some not condition logic to every condition logic(they are equivalent to each other) because it is easier to read.

References

Fixes #11842

Reviewer guidance

I have tested this with nested empty folders as well and this works on them too. However, I haven't tested them on completely empty folders with no content as I couldn't find such channel(or make one).


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

Also added every condition instead of some as I think this it is easier to read.
@github-actions github-actions bot added APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: frontend SIZE: small labels Mar 25, 2024
@MisRob
Copy link
Member

MisRob commented Apr 18, 2024

Thansk @Wck-iipi, we will have a look

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.

Hi @Wck-iipi, thank you for your contribution. While manually testing I noticed that you have only fixed the issue with the expanded filter but the issue with the expanded folders list is still extant and should be fixed as well. Once I select a folder, then I should first see the contents of the folder and then decide whether to further explore the contents of another folder:

2024-04-18_13-37-17.mp4

Signed-off-by: wck-iipi <21dcs006@nith.ac.in>

Fixed linting
@Wck-iipi
Copy link
Contributor Author

@pcenov I have fixed it. Please check if it is working correctly now.

@MisRob MisRob requested a review from pcenov April 25, 2024 03:37
pcenov
pcenov previously approved these changes Apr 25, 2024
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.

Thanks a lot @Wck-iipi - confirm that the filters are functioning correctly now!

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.

Hi @Wck-iipi - thank you for your work on this, and I'm sorry for the delay in code review!

Overall, it seems like the functionality is working as expected based on @pcenov 's review, so thank you. I do have one question about the conditional check you added. Thank you!

params: { ...route.params, id },
query: route.query,
});
if (children.every(c => c.title == '')) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain the nested if statements here? I understand the first part as the refactor, and I agree that it keeps the condition and makes it easier to read, so thank you. The second one, I'm hoping you could explain why you made this change. Is it to account for empty folders that you mention in the description? If so, I do think having a condition to manage this is a good idea, but I wonder if there might be something a bit easier to logic through than checking by the c.title. Were there other solutions or ideas that you had when thinking through this?

@rtibbles rtibbles dismissed pcenov’s stale review May 3, 2024 18:31

Still outstanding questions on the code review, so dismissing this until they are resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: frontend SIZE: small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants