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

Adds user suggestions in comment section #4094

Merged
merged 7 commits into from Jul 8, 2020

Conversation

SaptakS
Copy link
Contributor

@SaptakS SaptakS commented Jun 25, 2020

Before submitting pull request, please ensure that:

  • Every commit has a message which describes it
  • Every commit is needed on its own, if you have just minor fixes to previous commits, you can squash them
  • Any code changes come with test case
  • Any new functionality is covered by user documentation

@lgtm-com
Copy link

lgtm-com bot commented Jun 25, 2020

This pull request introduces 1 alert when merging f46796896f65b375eb90e9323e594db5f0d19e75 into 6ef7bc5 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

Copy link
Member

@nijel nijel left a comment

Choose a reason for hiding this comment

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

Have you looked at option of using some existing rich editor component, which would support autocompletion? We want to do more things in the editor....

weblate/api/views.py Outdated Show resolved Hide resolved
@SaptakS
Copy link
Contributor Author

SaptakS commented Jun 25, 2020

Have you looked at option of using some existing rich editor component, which would support autocompletion? We want to do more things in the editor....

I did. I was trying my best to implement it minimally. Also, most RTE don't have the @mention support. There is CKEditor which has @mention abilities. There is also Quill which doesn't have @mention directly but has additional npm plugins which can do it.

Seems like both the projects are active. Though Quill seems to have more adoption that ckeditor based on github "used_by" stat.

@SaptakS SaptakS force-pushed the user-suggestion branch 2 times, most recently from a8fa64a to e9a5497 Compare June 30, 2020 16:00
@SaptakS
Copy link
Contributor Author

SaptakS commented Jun 30, 2020

I have added the code to show the user suggestion. Need to still add tests. Not sure if documentation will be needed. I would rather add the test once we are sure and happy with the design and workflow.

@lgtm-com
Copy link

lgtm-com bot commented Jun 30, 2020

This pull request introduces 4 alerts when merging e9a5497ef33df65000614e74b91a20c92a8dba15 into 95fb213 - view on LGTM.com

new alerts:

  • 4 for Useless conditional

Copy link
Member

@nijel nijel left a comment

Choose a reason for hiding this comment

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

Please use yarn to install third-party libs and place them under weblate/static/vendor, see https://docs.weblate.org/en/latest/contributing/frontend.html

weblate/templates/translate.html Outdated Show resolved Hide resolved
weblate/templates/translate.html Outdated Show resolved Hide resolved
weblate/static/loader-codemirror.js Outdated Show resolved Hide resolved
weblate/static/loader-codemirror.js Outdated Show resolved Hide resolved
weblate/static/loader-codemirror.js Outdated Show resolved Hide resolved
weblate/static/loader-codemirror.js Outdated Show resolved Hide resolved
weblate/static/loader-codemirror.js Outdated Show resolved Hide resolved
weblate/static/loader-codemirror.js Outdated Show resolved Hide resolved
weblate/static/loader-codemirror.js Outdated Show resolved Hide resolved
@SaptakS SaptakS marked this pull request as ready for review July 6, 2020 15:19
weblate/static/loader-codemirror.js Show resolved Hide resolved
weblate/static/loader-codemirror.js Outdated Show resolved Hide resolved
weblate/static/loader-codemirror.js Outdated Show resolved Hide resolved
@nijel nijel added this to the 4.2 milestone Jul 8, 2020
@nijel nijel linked an issue Jul 8, 2020 that may be closed by this pull request
@nijel nijel merged commit 898beec into WeblateOrg:master Jul 8, 2020
@nijel
Copy link
Member

nijel commented Jul 8, 2020

Merged, thanks for your contribution!

nijel added a commit that referenced this pull request Jul 8, 2020
It is broken right now as it sens keys to textarea which is hidden
by CodeMirror, see #4094
@nijel
Copy link
Member

nijel commented Jul 8, 2020

Probably merged too fast - I thought tests were passing before, but apparently this broke Selenium tests. I've removed the extra test case in e7bb6e2 and fixed other one in a2c0fe7.

@nijel nijel self-assigned this Jul 8, 2020
@SaptakS
Copy link
Contributor Author

SaptakS commented Jul 8, 2020

Thanks for catching that.

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.

Comment mention autosuggestion
2 participants