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

Allow captcha when submit a webllink #223

Merged
merged 16 commits into from Jul 4, 2016

Conversation

alikon
Copy link
Contributor

@alikon alikon commented Jun 26, 2016

Pull Request for Issue #209 .

Summary of Changes

allow to use captcha when submit a weblink

Testing Instructions

use submit weblink enabling/disabling captcha settings

allow captcha  on submit web link (1/5)
allow captcha  on submit web link (2/5)
allow captcha  on submit web link (3/5)
allow captcha  on submit web link (4/5)
allow captcha  on submit web link (5/5)
@yvesh
Copy link
Member

yvesh commented Jun 26, 2016

@alikon Hey Nicola, works fine, but there are some small issues i found so far:

The tooltip text: Type in the textbox what you see in the image is not really matching for Recaptcha version 2 (where you don't fill out text boxes, but click images).

Some code style issues (see comments).

Thank you!

@@ -14,6 +14,17 @@
JHtml::_('formbehavior.chosen', 'select');
JHtml::_('behavior.modal', 'a.modal_jform_contenthistory');

$captchaEnabled = false;
$captchaSet = $this->params->get('captcha', JFactory::getApplication()->get('captcha', '0'));
foreach (JPluginHelper::getPlugin('captcha') as $plugin)
Copy link
Member

Choose a reason for hiding this comment

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

A new empty line is missing before the foreach loop

@chrisdavenport
Copy link
Contributor

A couple of issues:

  1. If I fail the captcha the page reloads but all the form field entries are lost.
  2. Perhaps move the captcha field to the end of the form?

CS fix
@alikon
Copy link
Contributor Author

alikon commented Jun 26, 2016

@yvesh

The tooltip text: Type in the textbox what you see in the image is not really matching for Recaptcha version

is like com_contact what should be the correct STRING ?

@chrisdavenport

  1. Perhaps move the captcha field to the end of the form?

not sure should be better near the submit button ?

  1. If I fail the captcha the page reloads but all the form field entries are lost.

any help ?

@yvesh
Copy link
Member

yvesh commented Jun 26, 2016

@alikon Not sure on the text. Something more generic maybe, which fits both.

For example: Please answer the security question. But i am not a native speaker. @chrisdavenport What do you think?

screenshot 2016-06-26 12 13 55

If I fail the captcha the page reloads but all the form field entries are lost.

any help ?

This is not going to be implemented easily. For com_users the data the user entered during registration is saved in the session and then loaded again.

See this code as an example.

@chrisdavenport
Copy link
Contributor

I checked with the en-GB team and because "I'm not a robot" is not a question, the suggested text is "Please complete the security check." There is a PR in the CMS repo for the same text: joomla/joomla-cms#10931

Good point about the submit buttons. Actually, I think the buttons should be moved to the end too. See, for example, the contact form.

Should be fairly simple to save the form data in the session as per com_users and also com_contact.

update captcha message like com_user, com_contatct #10931
handle session data 1(/2)
handle session data (2/2)
move buttons down
@alikon
Copy link
Contributor Author

alikon commented Jun 27, 2016

  • session form data
  • captcha & buttons moved down
    captchadown
    ....

@chrisdavenport
Copy link
Contributor

I have tested this item ✅ successfully on d6a18f8

Works perfectly and looks good. Thanks Nicola. 😄


This comment was created with the J!Tracker Application at issues.joomla.org/weblinks/223.

@alikon
Copy link
Contributor Author

alikon commented Jun 27, 2016

there is the need to add the lang string to the joomla-cms repo too ?
joomla/joomla-cms#10895 (comment)

@yvesh
Copy link
Member

yvesh commented Jun 28, 2016

@alikon There is a slight css issue for me:

screenshot 2016-06-28 16 44 40

Imo it's not completely related to your PR, even the


element has that issue. This is a clean Joomla! staging / protostar + weblinks installation.

Everything else works as expected, great work Nicola!

@alikon
Copy link
Contributor Author

alikon commented Jun 28, 2016

@yvesh css is not my "comfort zone"

any help to fix CSS issue ?

@alikon
Copy link
Contributor Author

alikon commented Jun 29, 2016

...it happens only when editor is CodeMirror...

@yvesh
Copy link
Member

yvesh commented Jun 29, 2016

@alikon it also happens without an editor. But you are right with TinyMCE it's fine.. Probably something which should be fixed in core (the row with buttons need to have width: 100% for the editor line, so there is no free space right of it).

We could fix it for weblinks with the following css:

#editor-xtd-buttons {
   width: 100%
}

@chrisdavenport What do you think? Core problem or weblinks?

screenshot 2016-06-29 21 34 38

@dgrammatiko
Copy link
Contributor

@alikon @yvesh how about: joomla/joomla-cms#10970

@alikon
Copy link
Contributor Author

alikon commented Jun 30, 2016

maybe @dgt41 can evaluate my dirty fix on https://github.com/joomla/joomla-cms/blob/staging/templates/protostar/css/template.css#L2609
from

.btn-toolbar {
    font-size: 0;
    margin-top: 9px;
    margin-bottom: 9px;
}

to

.btn-toolbar {
    font-size: 0;
    margin-top: 9px;
    margin-bottom: 9px;
        width: 100%;
}

results is clean now, i think...
capweblilnk

but i don't know if it is a goog fix .....

@yvesh
Copy link
Member

yvesh commented Jun 30, 2016

@alikon looks fine for me, but not sure if that would cause other issues though. You could also just remove the pull-left class the editor-xtd-buttons div has. Not sure why that is there at all?

Is there a case were there is something located right to it on the same line?

// cc @dgt41

@chrisdavenport
Copy link
Contributor

Any objections to merging this now?

@alikon
Copy link
Contributor Author

alikon commented Jul 4, 2016

For my knowledge of CSS the issue we have should be fixed on the CMS CSS side
We are waiting for some CSS expert answer which should be the right fix mine or Yves or ...
Evoking @dgt41

@yvesh
Copy link
Member

yvesh commented Jul 4, 2016

@alikon The branch now has conflicts (Probably due to Tobias changes related to code style).

I think we can merge this PR. The CSS layout problem is a minor one and only occurs when you have no editor or CodeMirror. We should open an issue at core for that. The css code from Nicola looks fine, but i am not sure if it has side effects.

@alikon
Copy link
Contributor Author

alikon commented Jul 4, 2016

yes please merge this PR

p.s.
cause i'm unable to find the latest damned branch conflicts 😭

@jissues-bot
Copy link

This PR has received new commits.

CC: @chrisdavenport

@yvesh
Copy link
Member

yvesh commented Jul 4, 2016

@alikon i sent you an PR against your one, merging current master changes into it. (Test please)

Merged with master from weblinks
@yvesh
Copy link
Member

yvesh commented Jul 4, 2016

Okay conflicts resolved. @chrisdavenport you can merge now.

@alikon
Copy link
Contributor Author

alikon commented Jul 4, 2016

thanks 1000 @yvesh

@jissues-bot
Copy link

This PR has received new commits.

CC: @chrisdavenport

@chrisdavenport
Copy link
Contributor

Merging. Good work everyone. :-)

@chrisdavenport chrisdavenport merged commit 1680ec7 into joomla-extensions:master Jul 4, 2016
@alikon alikon deleted the patch-4 branch September 6, 2016 07:02
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.

None yet

6 participants