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

Implemented Method to check page number #4841

Open
wants to merge 5 commits into
base: unstable
Choose a base branch
from

Conversation

Arunima22
Copy link

Summary

I have added a checkPageToLoad() Method in SearchResultsList.vue.
This method makes sure that fresh searches always load with Page One.

References

Here is the issue link: #4813 (comment)

Reviewer guidance

The reviewer, while trying to search for resources, should first search for a resource or keyword that will at least result in 11 search options and then navigate to page 2. Post navigating to page 2, the reviewer should search for something random that is likely to not produce any results. The fresh search will load with page one.

@Arunima22 Arunima22 changed the title Implemented Method to check page Implemented Method to check page number Nov 25, 2024
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.

I have no doubt this is working, but it might be simpler to change this at the source when initially navigate to the search page. See where that happens here - you will note that we are currently preserving most of the existing query of the route, instead we should update the page to 1 there: https://github.com/learningequality/studio/blob/unstable/contentcuration/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchOrBrowseWindow.vue#L178

I think you may be able to get this working with just a one line change there!

@rtibbles rtibbles self-assigned this Nov 26, 2024
@Arunima22
Copy link
Author

Hello @rtibbles that is an excellent suggestion. I shall change this by EOD. Thank you.

@Arunima22
Copy link
Author

Hello @rtibbles you may check now. Thanks for the suggestion.

@rtibbles
Copy link
Member

rtibbles commented Dec 5, 2024

Hi @Arunima22 - this is looking great, thanks. I'll double check the behaviour, but I am pretty sure this can be merged!

@rtibbles
Copy link
Member

rtibbles commented Dec 5, 2024

Just make sure to run this: yarn run lint-frontend:format and commit the results.

@Arunima22
Copy link
Author

@rtibbles yes okayy

@Arunima22
Copy link
Author

@rtibbles done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants