在使用这个模块的时候我发现第三方登录方式入口的图片替换起来似乎不是很方便。
因为在login表单的下方，所有第三方的入口并没有写上特殊的id或者class。
所以，我打了一个patch，这样给每个li标签里面都加上了id方便写样式。

CommentFileSizeAuthor
#7 csna_add-unique-html-id-2035983-7.patch806 bytesycshen
#3 csna_add-unique-html-id-2035983-3.patch789 bytesycshen
add-unique-id.patch790 bytesycshen

Comments

dydave’s picture

dydave commented
Status: Needs work » Needs review

Hi ycshen,

Thanks very much for submitting this feature request with a patch.

Patch code looks very good!

I haven't tested it to check the generated HTML/CSS markup, but would assume it would insert the correct ID for the corresponding provider in the HTML markup.

Several comments though that we would greatly appreciate, for the next time you submit a patch:

  1. Please submit your patch in the comments, not in the issue summary.
    This way, it would also trigger the testbot.
  2. Change ticket's status to needs review (so that maintainers and other users know there is a patch and the ticket needs to be reviewed).
  3. Patch naming convention, please check: Submitting patches:
    We would be expecting a patch named: csna-add-unique-html-id-2035983-1.patch
    (if you were to update the ticket in the first comment #1)

Otherwise, I wouldn't see any other issue that could prevent this patch from being applied to the 7-x.1-x development branch.
I would assume @alfababy should be able to get this committed after testing the patch and reviewing again the code.

Once again, thank you very much for your contribution.
Any additional feedback, testing, comments, issues, suggestions, or concerns on any aspects in this ticket or patch would be highly appreciated.

Thanks in advance to all for your comments, testing, reviewing and reporting.

Status: Active » Needs work

The last submitted patch, add-unique-id.patch, failed testing.

ycshen’s picture

ycshen commented
Status: Needs work » Needs review
StatusFileSize
new csna_add-unique-html-id-2035983-3.patch789 bytes

hi DYdave,

Thank you for your reply and proposal.

I have update my patch.

my patch is :

csna.module |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/csna.module b/csna.module
index 1039d1d..2cd67cf 100644
--- a/csna.module
+++ b/csna.module
@@ -251,7 +251,11 @@ function csna_providers($valid = FALSE) {
 function theme_csna_providers($providers) {
   $items = array();
   foreach ($providers as $provider_id => $provider) {
-    $items[] = l($provider['display_title'], 'csna/' . $provider_id, array('html' => TRUE));
+    $items[] =  array(
+      'data' => l($provider['display_title'], 'csna/' . $provider_id, array('html' => TRUE)),
+      'id'=>  $provider_id,
+    );
   }
-  return theme('item_list', array('items' => $items));
+
+  return  theme('item_list', array('items' => $items, 'attributes' => array('class' => 'csna-items')));
 }

before patched html is:

<ul >
<li class="first last">
</li>
</ul>

after use patch the html will be:

<ul class="csna-items">
<li id="qq" class="first last">
</li>
</ul>
dydave’s picture

dydave commented

Hi ycshen,

Thank you very much for your prompt reply.

Did you see how the first patch you submitted had a parse error?

I didn't catch this error, when reviewing:
return = theme('item_list', array('items' => $items, 'attributes' => array('class' => 'csna-items')));

so perhaps it is a little bit clearer to you now why the testbot would be so important for maintainers for testing the submitted patches....

Thanks for submitting an updated, correct patch (which was successfully validated by the testbot), and additional details about the results to expect after applying the patch, for the HTML markup changes.

Let's see if any other users could review and test this patch, maybe @alfababy.

Any additional feedback, testing, comments, issues, suggestions, or concerns on any aspects in this ticket or patch would be highly appreciated.

Thanks in advance to all for your comments, testing, reviewing and reporting.

ycshen’s picture

ycshen commented

:)

The testbot is very useful,i like it.

alfababy’s picture

alfababy commented
Status: Needs review » Needs work

Hi ycshen,

Thanks for your patch, looks it is very useful.

I think you can add prefix on the id, like "csna_provider_$provider_id".
Because we should ensuring that id attributes are unique on a Web page. (Reference)

Cheers.

ycshen’s picture

ycshen commented
StatusFileSize
new csna_add-unique-html-id-2035983-7.patch806 bytes

@alfababy
thank you for your reply.

i have updated the patch,please help review it again.

alfababy’s picture

alfababy commented

Commit it on 9912c56.

Thanks ycshen.

alfababy’s picture

alfababy commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.