Problem
At this moment we have two implementation for the same functionality: user_login and user_login_block
As I see there is no reason for that as they are mostly the same.
Solution
1. Removed user_login_block function
2. Removed redirect also mentioned in #1168514: Remove drupal_goto from user_login()
3. Links got named classes for better theming
4. Move links under "Log in" button:
===>>
Comment | File | Size | Author |
---|---|---|---|
#58 | 1772584-user_login_form-58.patch | 15.74 KB | gumanist |
#56 | 1772584-user_login_form-56.patch | 15.92 KB | gumanist |
#55 | 1772584-user_login_form-55.patch | 15.81 KB | andypost |
#55 | 1772584-interdiff-52vs55.txt | 2.52 KB | andypost |
#52 | 1772584-user_login_form-52.patch | 14.94 KB | gumanist |
Comments
Comment #2
andypost+1 to unify login form!
suppose this part should stay as is.
Comment #3
gumanist CreditAttribution: gumanist commentedre-rolled patch
Comment #4
gumanist CreditAttribution: gumanist commentedThis was removed to avoid conflicts with other forms. For example search page needs to have active search box, not login
Comment #6
gumanist CreditAttribution: gumanist commentedUpdated test SimpleTest's BrowserTest test case with different URL
Comment #7
andypostSuppose this links should be displayed conditionally. To do so just add a parameter for form builder
Comment #8
gumanist CreditAttribution: gumanist commentedAdded settings for #2 and #7 + some reordering in function.
Comment #9
podarok#8 looks good for me
RTBC ?
Comment #10
andypostmysterious hunk
is there no other way to return a render-array - see http://drupal.org/node/1777324
Should they be moved to user_block_configure()
Comment #11
andypostSettings for form are moved to block settings, defaults are mimic current core's
Test now is not affected.
Parallel issue which tries to kill the same redirect for form #1168514-6: Remove drupal_goto from user_login()
Comment #12
gumanist CreditAttribution: gumanist commentedLooks good, thank you for rerolling patch. Should we mark it as RTBC?
Comment #13
podarok#11 looks good!
thanks for reroll
Comment #14
catchCleaning this up is great, but why this?
This feature is something that can be easily done by contrib or custom code, I don't see that it's necessary to make these optional in core, or in general to add new admin interface elements here.
Comment #15
andypost@catch this settings should be added as follow-up patch. I think it could be a good addition for new block-plugins. So here's a patch without this
Comment #16
gumanist CreditAttribution: gumanist commentedAdded names to links array to allow clean altering
Added classes to links to have styling possibility
Comment #17
andypostSuppose this small enhancement should pass a gates
just adds a named class on this links! This should really improve styling out-of-box
Comment #18
catchSorry now there's settings without a UI:
Can we remove all this too and just keep the functionality the same as now?
Comment #19
andypost@catch no, this settings are required to configure a form:
1) display or not a links
2) set destination of form submit (probably could be removed) see also #1168514: Remove drupal_goto from user_login()
3) autofocus on user name is required for login page
Comment #20
catch@andypost - we don't have those settings now, so we don't need to add them in a refactoring issue. I'm OK with them being discussed/reviewed in a follow-up but as the patch stands it's adding additional code for something we don't even know if we want.
Comment #21
andypostThanx @catch for idea of form customization
1) Login form is customized within user_block_view()
2) Links are moved to separate renderable, and gets classes with theme_links()
Comment #22
gumanist CreditAttribution: gumanist commentedFrom my point of view it would be better to have most common items in place and it is up to user to use them or no. It is much easier to explain that get form with that parameters will lead you to that results.
For login it is:
User + password inputs and password reset + user register links.
But it is fine by me to move those updates as a follow-up issue. Let's mark it as RTBC?
Comment #23
podarokagree with follow-ups
#21 looks good for me
Comment #24
sunNeeds to be re-rolled due to #1538462: Cannot log in with OpenID due to "required" attribute
This could also use some more architectural reviews before it gets back to RTBC again.
I'd also like to see a follow-up issue to rename user_login() into user_login_form().
Comment #25
andypostMerged conflicts and changed back a way like links rendered
This change require usability review because I think that links (create account ...) should be placed after form - form inputs are grouped with submit button and links just additional fuctionality
Screens here
EDIT Yes, we need a follow-up to rename user_login() to user_login_form()
Comment #26
andypostCurrently it looks like
Comment #27
Bojhan CreditAttribution: Bojhan commentedInteresting, I think andy is right. It is a little weird that the closest action to two forms, is to create a new account. It should be the form.
Comment #28
andypostAlso I think that login form is important part of UI so let's get accessibility team here to!
Maybe this issue should be renamed to "Replace user_login_block() & user_login() with user_login_form()" ?
Comment #29
andyposttagging
Comment #30
Everett Zufelt CreditAttribution: Everett Zufelt commentedFrom what I understand this issue doesn't change the UI or functionality, just refactors code (didn't look at patch). If so, then there are no concerns from an accessibility perspective. If UI or functionality changes exist please update issue title.
Comment #30.0
andypostUpdated issue summary.
Comment #31
andypost@Everett Zufelt, Patch changes a login form - moves links under "Log in" button and changes a style of links
EDIT: probably this could affect this patch #1591806: Change block "subject" so that it's called a (admin_)label like everything else on the theme layer
Comment #32
Everett Zufelt CreditAttribution: Everett Zufelt commentedNeeds work for clear title and updated summary.
I don't understand what move links under button means, Can a more precise description of the change please be given?
Comment #33
andypostI've update summary (4) and added screenshots
Comment #33.0
andypostUpdated issue summary.
Comment #34
andypostPatch with moving links out of form
Comment #35
andypostAnother round
Comment #36
andypost@chx suggest to make rename right in the issue
Comment #37
andypostPatch for bot, let's see how many tests got broken
Comment #39
nod_Made the JS work with a single set of links below the forms.
Comment #41
andypostAdded @nod_ js and fixed tests
Comment #43
andypost#41: 1772584-user_login_form-41.patch queued for re-testing.
Comment #44
andypostPatch returns back #openid-login hash navigation
@nod_ please review the changes...
Comment #45
andypost@nod_ probably new openid-link require some styling because there's no href in link
{cursor: pointer}
Comment #46
nod_no, no we don't need this hash thing, it's horrible. It's just because you can't make a link from Drupal with an empty href (and that's legit in html5) since we're now creating it from JS there is no need for it.
The css needs some work to make it look like before, but the code shouldn't put the hash thing back.
And well, I've already said it, this link should be a button to begin with.
Comment #47
nod_Comment #48
sunThe fragment/hash usage actually looks interesting here. It means that you can carefully prepare and craft a link that goes directly to the OpenID login form instead of the regular; i.e.,
/user/login#openid
vs./user/login#user
(the latter being equivalent to no fragment/hash; would be great if the name would match the "provider", so #user instead of #nogo).Comment #49
nod_Isn't that why we have a
user/login/openid
? The use case seems pretty convoluted.Comment #50
andypost@nod_ so was there any reason to introduce this hash tags in openid patch? #1538462-39: Cannot log in with OpenID due to "required" attribute
I just trying to make no regression with current functionality.
The css code was already adjusted to make no visual regression - cos now we use the same link for show/hide forms the cancel link inherits openid-icon
Another question here - should we make register/request password links use theme_links() as I made in #21?
like this
Comment #51
nod_The hash thing was introduced in #160163: Login with OpenID link has mis-encoding of # a year ago without a solid supporting use-case.
Comment #52
gumanist CreditAttribution: gumanist commented- Link for OpenID moved to links list
- Updated JavaScript
- Removed theming on frontend side
Comment #53
gumanist CreditAttribution: gumanist commentedfor bot
Comment #54
andypostGoing to make manual testing
I think we could skip this in block_user_view() and set in openid
EDIT RTL styles needs to be ajusted too
Comment #55
andypostChanges:
- fixed RTL-styling
- moved #weight for links to openid
- removed unused $errors variable in JS
- add focusing on openID input after switching
Comment #56
gumanist CreditAttribution: gumanist commentedre-rolled, because of
http://drupal.org/node/1168514
Comment #58
gumanist CreditAttribution: gumanist commentedRe-rolled again
Comment #59
andypostI think it's ready, let's get this in
Comment #60
podarok#58 +1 RTBC
Comment #61
webchickHm. There's an awful lot going on in this patch for such a simple-sounding title. I checked it over and the non-JS parts seem fine (I'm not sure how to evaluate the JS parts), and this has been sitting at RTBC for a few days now though, and I haven't seen anyone pipe up about it, so...
Committed and pushed to 8.x. Thanks!
Comment #62
EclipseGc CreditAttribution: EclipseGc commentedFollow up issue to this to clean up how it was implemented: #1826452: Do not alter forms after calling drupal_get_form()
Comment #63.0
(not verified) CreditAttribution: commentedUpdated issue summary.