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

Fixed: search jobs modal showing 'no jobs found' without searching any job (#85zrmar8z) #364

Merged
merged 7 commits into from
Feb 8, 2023

Conversation

alsoK2maan
Copy link
Contributor

Related Issues

Closes #358

Short Description and Why It's Useful

The select jobs modal showed 'No jobs found' without even searching for a job. Fixed the issue and now the modal shows 'Searched jobs will appear here' if no jobs are searched.

Screenshots of Visual Changes before/after (If There Are Any)

Job Manager - HotWax Commerce

IMPORTANT NOTICE - Remember to add changelog entry

Contribution and Currently Important Rules Acceptance

@alsoK2maan alsoK2maan requested a review from disha1202 February 6, 2023 11:21
Comment on lines 15 to 19
<div v-if="!isJobSearched" class="ion-text-center">
<p>{{ $t("Searched jobs will appear here") }}</p>
</div>

<div v-else-if="jobs.length === 0" class="ion-text-center">
Copy link
Contributor

Choose a reason for hiding this comment

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

Use queryString instead of isJobSearched

Copy link
Contributor Author

@alsoK2maan alsoK2maan Feb 7, 2023

Choose a reason for hiding this comment

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

As queryString is v-modelled with the search bar, even if we type something without searching (pressing enter) it'll show 'No jobs found' in that case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using v-model, we could update the queryString only on enter press. This way we will not need flag variable

@@ -12,7 +12,11 @@
<ion-content>
<ion-searchbar v-model="queryString" :placeholder="$t('Search jobs')" @keyup.enter="search($event)" />

<div v-if="jobs.length === 0" class="ion-text-center">
<div v-if="!isJobSearched" class="ion-text-center">
Copy link
Contributor

Choose a reason for hiding this comment

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

Also we can remove this.

Comment on lines 15 to 19
<div v-if="!isJobSearched" class="ion-text-center">
<p>{{ $t("Searched jobs will appear here") }}</p>
</div>

<div v-else-if="jobs.length === 0" class="ion-text-center">
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using v-model, we could update the queryString only on enter press. This way we will not need flag variable

@@ -92,7 +96,9 @@ export default defineComponent({
methods: {
async search(event: any) {
this.queryString = event.target.value.trim();
if(this.queryString.length > 0) this.getJobs();
if(this.queryString.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need the length check at all?

"That is, all values are truthy except false, 0, -0, 0n, "", null, undefined, and NaN."

https://developer.mozilla.org/en-US/docs/Glossary/Truthy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sir. The empty queryString itself is a false value.

<p>{{ $t("Searched jobs will appear here") }}</p>
</div>

<div v-else-if="jobs.length === 0 && queryString.length != 0" class="ion-text-center">
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the condition queryString.length != 0" might not be needed, else-if will execute only when queryString.length === 0 fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improved.

@adityasharma7 adityasharma7 merged commit c4a3103 into hotwax:main Feb 8, 2023
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.

Bulk Job scheduling: 'No jobs found' comes when we open the select jobs link
4 participants