-
Notifications
You must be signed in to change notification settings - Fork 267
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
Pure JS implementation #457
Open
raziel057
wants to merge
8
commits into
lexik:master
Choose a base branch
from
raziel057:pure-js
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
a39bfad
First release of Pure JS implementation
e4839f3
Fix deprecation (missing return type)
1ea8f88
Fix returning all data when no result found
8429805
Add no result info bar
56a539c
Fix tests for added deletion confirmation message
99452f1
Revert "Fix returning all data when no result found"
385797f
Revert "Fix deprecation (missing return type)"
db9a413
Fix issues and simplify code
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid to return all results (because there is no where condition added) when there is no ids matching the search. In case there is no matching results I add a condition to the builder that is never true.
With the fix when the search doesn't matches, we have an empty list:
![image](https://private-user-images.githubusercontent.com/652505/343191040-29dc9102-be16-40d7-b45e-a210abbfdfa7.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk2MDQ3MzgsIm5iZiI6MTczOTYwNDQzOCwicGF0aCI6Ii82NTI1MDUvMzQzMTkxMDQwLTI5ZGM5MTAyLWJlMTYtNDBkNy1iNDVlLWEyMTBhYmJmZGZhNy5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjE1JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxNVQwNzI3MThaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT04MjZiOTBlMjljOTZjOWFmZmE3ODhhYjRhYTg5NDdkOWYxNzRhNzVlMTU1NDYxNjg5NTRiOWQyNGI2YjZhY2M2JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.ovBCxsf1fyxBo-OOVzFxZjzHzAt9pNDYYu4H8UcFE9A)
We didn't wanted to throw a NoResult exception as we are in
addTranslationFilter
and didn't wanted to change to much the logics but if you have a better idea, please suggest it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be in a separate PR if unrelated to the pure JS implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Seb33300 Right, I just revert 2 unrelated commit (this fix and the fix of deprecation)