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

QR code History case sensitive #339

Merged
merged 2 commits into from
Apr 10, 2019
Merged

QR code History case sensitive #339

merged 2 commits into from
Apr 10, 2019

Conversation

ashish0910
Copy link
Contributor

This pr solves the issue mentioned here
Earlier the history content was case insensitive but QR code were case sensitive . In this PR history is also made case sensitive
Current:
current
Expected:
expected

Copy link
Owner

@llaske llaske left a comment

Choose a reason for hiding this comment

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

Removing the lowerCase call is not the solution

@@ -38,7 +38,6 @@ define(["sugar-web/activity/activity","sugar-web/datastore", "sugar-web/env", "w
var generateCode = function(text) {
qrCode.clear();
qrCode.makeCode(text);
var text = userText.value.toLowerCase();
Copy link
Owner

Choose a reason for hiding this comment

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

I think removing the toLowerCase is not the solution else the following line will not be able to detect string like "HTTP://" or "HttPs://". The solution is just to put the raw value in history.

@Scar26
Copy link
Contributor

Scar26 commented Apr 10, 2019

I was actually working on this.

Code_of_Conduct

Anyway, as lionel mentioned, removing the lowercase condition isn't the answer.

@ashish0910
Copy link
Contributor Author

I was actually working on this.

Sorry @Scar26 if that was the case . However from next time please let other fellow contributors know that you are working on an issue as you did not mention anywhere that you are working on this and I did not found any PR from your side to solve that issue .
Happy coding

@ashish0910
Copy link
Contributor Author

@llaske it should work now

@llaske llaske merged commit ae286c7 into llaske:dev Apr 10, 2019
@llaske
Copy link
Owner

llaske commented Apr 10, 2019

Nice. Thanks.

@quozl
Copy link

quozl commented Apr 10, 2019

@ashish0910, @Scar26, thanks for your contribution. Yes, it can be helpful to signal that you are working on something, but that does not stop anyone else from working on the same thing. Signal is usually not mandatory. What matters most is the problem is fixed. The value decisiveness part of the code of conduct.

For trivial fixes, the investment is small, the solution obvious, and immediately accepted. So the chances of a sense of wasted effort is lower.

For complex fixes, the investment can be large, the solutions diverse, and as a result maintainers (such as @llaske) may pick from more than one person. Again, the chances of a sense of wasted effort is lower.

In the open source environment, we expect your changes to be copied by others.

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.

4 participants