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

Bugfix/63 caret moved to the end #64

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

andygi
Copy link

@andygi andygi commented Apr 4, 2019

Bugfix of #63 (Caret moved to the end of input after change).
Fixed for LTR only and testable manually only, by index.html and jquery.html.
In the ticket there a clear description of how to reproduce the issue and, consequently how to test it.
I have created a jquery and javascript index RTL page in order to test RTL as well.

Copy link
Collaborator

@ddayguerrero ddayguerrero left a comment

Choose a reason for hiding this comment

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

I haven't properly tested the fix yet but the implementation looks good. Do you mind un-staging all changes in the dist/ folder please?

Thumbs up on the RTL version of the test page! 👍

package.json Outdated
@@ -28,7 +28,7 @@
},
"devDependencies": {
"browserify": "^16.2.3",
"bundle-collapser": "~1.1.1",
"bundle-collapser": "^1.3.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any specific reason why we're bumping the version for bundle-collapser?

Copy link
Author

Choose a reason for hiding this comment

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

not really, it is automatically updated during the installation.
I have reverted it.

@andygi
Copy link
Author

andygi commented Apr 8, 2019

@ddayguerrero,

Do you mind un-staging all changes in the dist/ folder please?

Could you help me to better understand what do you need, please?

Copy link
Collaborator

@ddayguerrero ddayguerrero left a comment

Choose a reason for hiding this comment

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

Sorry if I was unclear, you do not need to include the /dist folder for this pull request. We'll compile and minify everything once the new update is ready for publication.

I tested out the changes with the LTR index page and everything works well. RTL still loses its position but that behaviour is still the same on master, so we can fix it separately.

@andygi
Copy link
Author

andygi commented Apr 9, 2019

@ddayguerrero , I see, no problem I have reverted the code into dist directory and thank you for the explanation.
To be bulletproof, if you want, I can add this directory into gitingore file as well.
Let me know.

@ddayguerrero
Copy link
Collaborator

If you don't mind then that would be cool @andygi!

@andygi
Copy link
Author

andygi commented Apr 12, 2019

If you don't mind then that would be cool @andygi!

@ddayguerrero Sure, I have done!

@jondavidjohn
Copy link
Owner

jondavidjohn commented Apr 12, 2019

Sorry, but we actually want to not ignore that directory, we need it for package managers (like bower) that require we commit the compiled files.

https://github.com/jondavidjohn/payform/blob/master/bower.json#L3

@ddayguerrero
Copy link
Collaborator

Right, I completely missed that @jondavidjohn! Thanks for 👀, we can undo that change @andygi

@andygi
Copy link
Author

andygi commented Apr 12, 2019

@ddayguerrero , no problem I have reverted the commit.

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.

3 participants