This issue is the obvious followup to #1446650: After installing module display link to "install another module".
Problem/Motivation
Make installing another theme easier by linking to the install page after a new theme is installed.
Proposed resolution
Add a "Download another theme" "Install another theme" link after a new theme is installed.
Also, to match setup of core/lib/Drupal/Core/Updater/Module.php
, set up row of links in the following order:
- Install another theme
- Enable newly added theme
- Administration pages
Remaining tasks
Reviewed and Tested by the Community.
From core/lib/Drupal/Core/Updater/Module.php
:
/**
* {@inheritdoc}
*/
public function postInstallTasks() {
// Since this is being called outsite of the primary front controller,
// the base_url needs to be set explicitly to ensure that links are
// relative to the site root.
// @todo Simplify with https://www.drupal.org/node/2548095
$default_options = [
'#type' => 'link',
'#options' => [
'absolute' => TRUE,
'base_url' => $GLOBALS['base_url'],
],
];
return [
$default_options + [
'#url' => Url::fromRoute('update.module_install'),
'#title' => t('Install another module'),
],
$default_options + [
'#url' => Url::fromRoute('system.modules_list'),
'#title' => t('Enable newly added modules'),
],
$default_options + [
'#url' => Url::fromRoute('system.admin'),
'#title' => t('Administration pages'),
],
];
}
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Update the issue summary | Instructions | Done | |
Manually test the patch | Novice | Instructions | Done |
Embed before and after screenshots in the issue summary | Novice | Instructions | Done |
User interface changes
Before:
After:
API changes
None
Comment | File | Size | Author |
---|---|---|---|
#109 | Screen Shot 2017-01-28 at 5.25.58 PM.png | 66.9 KB | mspangenberg |
#107 | 1862094-2.patch | 1.75 KB | mspangenberg |
#106 | Screen Shot 2017-01-28 at 4.40.40 PM.png | 75.86 KB | mspangenberg |
#104 | 1862094 - after.png | 37.58 KB | steverossnyc |
#104 | 1862094 - before.png | 34.73 KB | steverossnyc |
Issue fork drupal-1862094
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
David_Rothstein CreditAttribution: David_Rothstein commentedAnd an obvious (but untested) patch.
Comment #2
benjifisherI tested it, and it works as expected. Luckily, there are a few modules with 8.x-?.x-dev releases available for experimentation. I did not try enabling the themes after downloading them.
I think being able to install multiple themes is less valuable than being able to install multiple modules, but this patch is good for consistency.
Shameless plug: my patch at #720452: Allow ability to install multiple modules at once could use some attention.
Comment #3
catchI'm not sure it's that obvious. Most sites have dozens of modules installed, but only one theme.
Comment #4
David_Rothstein CreditAttribution: David_Rothstein commentedWell, if you are downloading themes from drupal.org (rather than building your own) most likely you will want to try out a few before settling on a final one.
Comment #5
benjifisherI marked #1012850: Add "install another new module" link to "Next steps" after a new module has been installed as a duplicate of #1446650: After installing module display link to "install another module" and this issue.
Comment #8
benjifisherThe patch no longer applies. This should be an easy re-roll, so I am adding tags.
Comment #9
alimac CreditAttribution: alimac commentedRe-rolled.
Comment #10
alimac CreditAttribution: alimac commentedLooking at these two options, they're a bit confusing ("Install another theme vs. Install newly added themes"). Does "Install newly added themes" mean "Enable"?
Comment #11
alimac CreditAttribution: alimac commentedDid not apply to HEAD, so I rerolled it again.
Comment #12
benjifisherThe issue summary points out that this is a follow-up to the corresponding issue for installing modules, #1446650: After installing module display link to "install another module". The language used there is
I agree with the suggestion in #10 of changing the text on the existing link from "Install" to "Enable".
Comment #13
alimac CreditAttribution: alimac commentedHere's an updated patch which makes the link name clear and consistent.
Comment #14
benjifisher@alimac:
The interdiff does not look right. Can you check it and post again? Specifically, it is not consistent with the updates from #9 tp #11, replacing
l()
with\Drupal::l()
and replacing D7-style paths with D8-style routes.Comment #15
alimac CreditAttribution: alimac commented@benjifisher: the interdiff is for 11 to 13. I did not provide an interdiff for 9 to 11 because it was a reroll (I rebased, then resolved the conflicting lines).
Comment #16
benjifisher@alimac:
Sorry, I must have been looking at the wrong interdiff.
I am marking this RTBC. I already did that way back in #2. The only objection has been @catch's comment in #3. This is not at all a technical question, it is a UX question. In my opinion, consistency with the modules page is a good enough reason to include this patch. (Even though I am not at the Amsterdam code sprint, I took the trouble to pull the latest code from git and check that it is, indeed, consistent.)
Comment #18
alimac CreditAttribution: alimac commentedRerolled for latest HEAD.
Comment #19
benjifisher@alimac:
It is hard to hit a moving target! The patch still looks good to me.
See my comments in #16.
Comment #20
alimac CreditAttribution: alimac commentedComment #21
alimac CreditAttribution: alimac commentedComment #22
alexpottHmmm... the new link here is wrong. It should be to update.theme_install. Also I think we have a problem with what install means. Since what you can do on that route is download a new theme. And all the usage of the word "enable" has been removed.
Comment #23
kallehauge CreditAttribution: kallehauge commentedI agree with @alexpott at #22 and suggest that we keep the original link that was modified in patch #18 by @alimac and simply add a new link pointing to "update.theme_install" with the string "Download another theme".
Comment #24
alimac CreditAttribution: alimac commented@kallehauge: thanks for posting a new patch. I'm changing the status of the issue to Needs review so that the testbot can automatically test it.
Comment #25
alimac CreditAttribution: alimac commentedThis is a small patch, so it's easy to "eyeball" the difference, but it's really helpful to post an interdiff between a patch and the previous one, which shows the difference between them. Here's how to make one (for future reference): https://www.drupal.org/documentation/git/interdiff
Comment #26
kallehauge CreditAttribution: kallehauge commentedI was actually about to post an interdiff, but considering the amount of code that was changed in the patch, I actually felt it would be more confusing than helpful in this scenario :) But I will keep it in mind! Thanks @alimac!
Comment #29
vlad.n CreditAttribution: vlad.n commentedinstall-another-theme-1862094-23.patch patch applied cleanly at f07bcff.
Comment #30
rpayanmTestbot random failures :(
Comment #33
alimac CreditAttribution: alimac commentedI think we might have to mark this as postponed, due to: Remove the UI for installing/updating modules from update module if it is not fixed in time for release.
Installing themes (and modules) through the web UI doesn't work currently, so the patch for this issue cannot be tested.
Comment #34
alimac CreditAttribution: alimac commentedComment #35
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis is marked for Drupal 7 backport so I don't think it should be postponed.
To test it manually on Drupal 8, just apply it on top of the latest patch from #2042447: Install a module user interface does not install modules (or themes).
Comment #37
kfitz CreditAttribution: kfitz at Acro Commerce commentedWhile attempting to install themes to test this patch, on a virtual host, I kept receiving the error: 'theme_name' was not found. This was case even when attempting install the following:
Adminimal Theme
Omega Theme
Adaptive Theme
I was able to successfully apply the following two patches:
https://www.drupal.org/node/2042447
#23
I set permissions on themes to 777 and unpacked the themes into the themes folder after downloading them. Additionally, I was not able to install themes via providing a url or path to a zip/tar. I'm not sure if there is something I'm missing, or the D8 API is not stable enough to manually test this issue, but it would be nice to see it make it into 8.0.x.
Comment #38
rutgers03 CreditAttribution: rutgers03 commentedWorking on this at Drupal Camp NJ
Comment #39
rutgers03 CreditAttribution: rutgers03 commentedComment #40
swetashahi CreditAttribution: swetashahi at Srijan | A Material+ Company commentedUable to test using the patch in #23 in simplytest.me. Got an error saying "An error occurred while patching the project." Marking Needs work
Comment #41
hugronaphor CreditAttribution: hugronaphor as a volunteer commentedHere is the path applicable to the latest 8.0.x and 8.1.x branches.
No interdiff as core implementation changed since the previous patch.
Comment #42
hugronaphor CreditAttribution: hugronaphor as a volunteer commentedTriggering testbot.
Comment #43
swetashahi CreditAttribution: swetashahi at Srijan | A Material+ Company commentedI tested the patch in #41 using SimplyTest .me on chrome. here are the steps followed while testing:
1. Downloaded theme from drupal.org site from https://www.drupal.org/node/2638078
2. Go to Appearance -> Install a new theme
3. Browse to the downloaded zip file. Click Install
4. Installation happens successfully and the following message appears under "Next steps"
"Download another theme
Install newly added themes
Administration pages"
Screenshot here
Clicking on Download another theme takes to the Install new theme screen. Here the newly installed theme in step 3, (surprisingly) appears under uninstalled themes but again clicking install moves it to installed themes section in a second.
This is as per the proposed resolution. Marking RTBC
Comment #44
catchIt seems more likely that you'd want to install the theme you just downloaded than go install another one - so shouldn't this be the second rather than first option in the list?
Comment #45
swetashahi CreditAttribution: swetashahi at Srijan | A Material+ Company commentedBut in the ticket description the proposed resolution was having a link for Installing another theme.
If the issue is about the ordering of text then it should be a separate ticket.
Comment #46
catch@swetashahi it's adding the link where there was previously no link and introducing the ordering issue by doing so - that makes it a problem with the patch here, not a separate issue.
Comment #47
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI think this should be made exactly consistent with how the module links work (which is what I tried to do in the original patch in #1).
Since the module links are currently:
The theme links should be:
If we want to reorder the links it needs to be done for modules too, but I think it might be a bit out of scope for this issue.
And "Download another theme" seems like it would be pretty confusing - what you're doing from this page is actually much closer to "uploading" than "downloading".... There are already other issues in the queue that talk about changing the terminology around this, so I don't think we need to get into it here.
Comment #48
Manjit.SinghHere is my observations Guys.
I have test the latest patch. Please check attached screenshots.
Before
After
@Catch The position/placement of Download link is at top. And we have to modify the Issues description a bit for better understanding as @David mentioned above.
Or Can we create a separate ticket for it ?
Comment #49
Manjit.Singh@David reordering links as per your feedback.
Comment #50
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThanks - I didn't test the new patch, but from looking at it aren't they still in an inconsistent order compared to the links for modules?
Comment #52
Truptti CreditAttribution: Truptti at Axelerant commentedVerified the patch ' install-another-theme-1862094-49.patch'' in #49 on drupal 8.1.x, also the same patch applied successfully on drupal 8.2.x site .After installing theme the links on success screen are displayed as below
Attaching snapshot for reference.
Comment #53
Truptti CreditAttribution: Truptti at Axelerant commentedComment #54
catchSites are expected to have tens of modules installed, sometimes into the hundreds.
They're only expected to have two themes, more than that is for extremely rare special cases.
So these are not equivalents, and treating non-equivalents the same is not something we do elsewhere - i.e. the modules and appearance pages are not the same either.
Tagging 'Needs usability review' and for product review, but I'm not personally going to commit this as is.
Comment #55
yoroy CreditAttribution: yoroy at Roy Scholten commentedI did raise my eyebrows at this for the reasons @catch gives. It's not at all obvious to me that because the modules screen has this then appearance page should have it too. It would be less of a problem if we had stronger visual hierarchy among these links, we'd make the "install" link the primary one and the other two would serve the less important scenarios. Because we don't have that hierarchy here this link adds friction: choosing between 3 instead of 2 links is more work for the user. For those who do want to install the next theme right away, it's only one more click to the Appearance page (via "install newly added" link) where you can choose to "install new theme" again.
So, this is a worthwhile shortcut if installing multiple things is the more likely scenario. That does apply to modules, but not to themes. Lets not do this.
(I'm collecting visual hierarchy issues under the "needs design" tag)
Comment #56
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThe screenshots in #52 show that the patch still puts this link in the wrong place, so marking "needs work" for that. (We should create a separate issue to discuss whether "enable" belongs before "install" more generally, i.e. in the module case.)
I really don't understand why people think this link wouldn't be useful. To repeat my comment from #4:
That really seems like the most likely scenario to me - you've found a few themes that look potentially interesting and you want to download and try them all out.
On top of that there's the issue of base themes - a decent number of contrib themes depend on a base theme, so if you're downloading one of those then even if you do already know that's the final theme you want to use you'll still need to go back and download a second one.
Yes, it's only one extra click, but I'm not sure it's an obvious click for new users.
Comment #57
saurabhsirdixit CreditAttribution: saurabhsirdixit commentedAltered the links as per comment #56 to make similar experience with module screen.
Comment #58
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedEmpty spaces here. I don't know are you placing it on correct place.
Comment #59
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedComment #60
jfraz CreditAttribution: jfraz commentedI spun this up on simplytest.me and verified that the link indeed exists and directs the user to the page to install another theme.
Comment #61
jfraz CreditAttribution: jfraz commentedI've reviewed this and feel it works as intended. I normally build my own themes, so this isn't something I would use. But I do see the applicable use for someone who is testing various themes. Updated issue summary. This was reviewed for training at DrupalCon New Orleans.
Comment #62
jfraz CreditAttribution: jfraz commentedComment #63
hugronaphor CreditAttribution: hugronaphor as a volunteer and at Acrosto for Acrosto commentedPlease stop testing or uploading new patches for this issues.
Someone needs to decide either we need this "install another theme" link or not!
See: #54 and #55
Comment #64
catchYoroy already weighed in. But putting back in the usability queue one more time, and leaving the product review tag on as well.
Comment #66
Manjit.Singh@catch @codethenode the link "Enable newly added themes" is taking the user to appearance page to enable the particular theme. But as per the usability, the link should enable the newly added theme rather redirecting the user to appearance page.
Comment #67
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commented@manjit right now how are you enabling theme.
You have to first go to the appearance page and then enable it. I think it is ok.
Different way of enabling theme either use drush or drupalconsole.
thanks
Comment #68
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #69
yoroy CreditAttribution: yoroy at Roy Scholten commentedWell, without the link for "install another" there's only two options, either go to the Appearance page or to the top level admin page :-)
I wouldn't be so against this if we could visually promote the "Enable newly added" one:
Edit: Forgot to mention that base theme + sub theme is a good argument in favor of adding the link.
Comment #70
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI like it, but the button makes it look like clicking on it will actually enable the theme (or at least take you to a dedicated page for doing that), so maybe it should be combined with or postponed on #992190: Link to enable a new module after adding it via the Update Manager is confusing - allow users to enable it directly from that screen instead? But one way or another it is definitely a good idea to highlight that action over the others.
Comment #71
yoroy CreditAttribution: yoroy at Roy Scholten commentedYou're right, this would not the correct styling, forgot to mention that. I don't think we currently have a primary link (not button) style, but we can make one.
Comment #72
Bojhan CreditAttribution: Bojhan as a volunteer and commentedLets have product manager review, before moving further on this patch to avoid wasting time.
Comment #73
webchickThis is like a 1% screen. I don't think it needs product management review.
However, my 2 cents is that #69 seems like it'd make sense for both modules and themes, and would both address David's concern about inconsistency, and catch's concern about promoting a non-default action as the default.
Comment #75
joelpittet@yoroy Maybe you could re-title this issue to re-focus it? It seems like there is agreement with the proposed new link suggestion.
Comment #76
joelpittetbat signal
Comment #77
lomasr CreditAttribution: lomasr at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commentedHi,
As suggested in #69 , I have created a patch. Please review.
Thanks
Comment #79
bhavikshah9 CreditAttribution: bhavikshah9 at Tatvasoft commentedPatch#77 failed. As well as, I guess, indentation is in-correct.
Comment #80
Thew CreditAttribution: Thew at Google Code-In commentedThe patch from #77
Comment #81
Thew CreditAttribution: Thew at Google Code-In commentedComment #83
Thew CreditAttribution: Thew at Google Code-In commentedFix more indentation.
Hope it pass.
Comment #85
benjifisherDid you test the patch at all? I would be surprised if it works.
You need to move the assignments and the
setOptions()
lines outside thereturn
statement.Also, this patch mixes the old
array('foo')
syntax with the newer['bar']
syntax. According to the coding standards,Comment #86
benjifisherI think a cleaner patch (untested) would be to assign
outside the
return
statement. Then, inside the array being returned, update the link text and applysetOptions($button_style)
to the appropriate URL.Comment #88
manishmittal9 CreditAttribution: manishmittal9 commentedComment #89
manishmittal9 CreditAttribution: manishmittal9 commentedComment #90
manishmittal9 CreditAttribution: manishmittal9 commented1862094-88.patch Updated Patch
Comment #91
manishmittal9 CreditAttribution: manishmittal9 commentedForgot to upload the patch in earlier comment.
Comment #92
cleverhoods CreditAttribution: cleverhoods as a volunteer commentedI'm going to review it
Comment #93
ifrikmanishmittal9,
Could you post an interdiff as well? That makes the reviewing easier.
https://www.drupal.org/documentation/git/interdiff
Comment #94
ifrikComment #95
cleverhoods CreditAttribution: cleverhoods as a volunteer commentedAfter applying the patch to a clean 8.4x installation I get the following error.
Comment #96
kusalavan CreditAttribution: kusalavan as a volunteer commentedI was able to test this successfully and i could see the "Install another theme" link after installing a theme.
Comment #97
ifrikkusalavan,
cleverhoods had put this issue on "needs work" because he encountered the error message above after applying the patch, and he is working on this during today's sprint.
Comment #98
cleverhoods CreditAttribution: cleverhoods as a volunteer commentedFixed the issues reported in #86. Please review the patch
Comment #99
cleverhoods CreditAttribution: cleverhoods as a volunteer commentedComment #100
ikit-claw CreditAttribution: ikit-claw as a volunteer commentedWorks fine on my setup php7
Could possibly do with a link to install a theme from appearance as well as extend?
Comment #101
cleverhoods CreditAttribution: cleverhoods as a volunteer commentedAdded it as a link instead of a button.
Comment #102
ikit-claw CreditAttribution: ikit-claw as a volunteer commentedUpdated no longer displayed as button
Comment #103
ikit-claw CreditAttribution: ikit-claw as a volunteer commentedComment #104
steverossnyc CreditAttribution: steverossnyc as a volunteer and commentedWorks on 8.4-dev
Before:
After:
Comment #105
steverossnyc CreditAttribution: steverossnyc as a volunteer and commentedComment #106
mspangenberg CreditAttribution: mspangenberg as a volunteer commentedIn order to maintain consistency with the Modules Next Steps the order of the operations should be:
See attached image.
Comment #107
mspangenberg CreditAttribution: mspangenberg as a volunteer commentedUpdated patch to match verbiage of Modules page.
Did not unit test. Appears functional based on prior test.
Comment #108
mspangenberg CreditAttribution: mspangenberg as a volunteer commentedAdded 1862094-2.patch and Screen Shot 2017-01-28 at 4.40.40 PM.png to resolve order of operations issue in verbiage.
Next steps for themes should mirror order of operations of Next steps for modules (see Screen Shot).
Comment #109
mspangenberg CreditAttribution: mspangenberg as a volunteer commentedTested 1862094-2.patch, screen shot attached, applied successfully.
Comment #110
cleverington CreditAttribution: cleverington as a volunteer commentedReviewed patch and tested.
Updated Issue Summary with new screenshot from mspangenberg.
Comment #111
cleverington CreditAttribution: cleverington as a volunteer commentedUpdated Summary for required alterations
Comment #112
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis goes against what was discussed in #69 (and what subsequent patches tried to implement). There is no visual distinction between the primary link and the other links.
Since we don't have a primary link styling (see #71) we need to come up with one. I am going to suggest simply making the link bold (i.e.
<strong>
tags) if no one else has a better idea...Also the screenshots in #104 and #110 differ and it's not clear which patch is RTBC. The #104 screenshot looks more like #69 (although still missing the styling) - I think the idea is to go with that here, and then ensure that installing a module uses the equivalent wording, link order, and styling as well.
Comment #113
cleverington CreditAttribution: cleverington as a volunteer commentedI would hesitate to update the UX/UI on
admin/appearance
when not making a similar alteration on the same page/functionality withinadmin/modules
.In truth, this issue appears to be a sibling of https://www.drupal.org/node/1446650
I agree that the ideas discussed in #69 / #71 are a good approach, but I think adding
<strong>
or an image-button would probably be a separate issuewith a relation to https://www.drupal.org/node/992190 as an additional UI/UX enhancement.The patch reviewed is https://www.drupal.org/files/issues/1862094-2.patch, an small tweak to https://www.drupal.org/files/issues/1862094.patch with UI matching for https://www.drupal.org/node/1446650.
The Screenshot was updated in the Summary.
If going forward with
<strong>
to re-enforce the primary link, I would recommend creating a sibling issue (unless one already exists I did not find)to https://www.drupal.org/node/992190and adding the button on both pages at the same time.Edit: Updated after reading background behind #9292190.
Comment #115
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI mostly agree with you - in fact it's what I suggested on this issue about a year ago :)
However, this new patch has the exact same change that was already rejected before. The UX maintainers felt it would be confusing to add the extra link without also making additional changes.
Comment #116
cleverington CreditAttribution: cleverington as a volunteer commentedAh. I See. #47. Rejected within #54.
So if the recommended alteration is not approved, should the Issue be Closed and a new issue created with the ideas behind #69, #71, and #73? Or altered to support those posts? Closed because neither option is viable Use Cases?
For continuance, it seems like it should be possible to create a small View which loads the content of
admin/appearance
within a container (or similar) with only the recently installed theme/themes.An array could track the associated theme(s) that were just installed. Then, upon installation, the User could Enable one of them before being redirected to the
admin/appearance
screen.Oddly, thinking about it, the fix seems more.... viable for the modules page altogether, simply because downloading and installing a theme could potentially download multiple themes, such as with Omega, etc. Trying to have the simplest method, a button which 'enables the theme' would fail simply because the array would always be more than [0].
In contrast, on the Modules page, completion would not be much of an issue, because the Module-Filter module is already an existing functionality and replicating the layout would be fairly easy.
Comment #123
jhodgdonAdd meta issue as parent
Comment #124
jhodgdonFYI -- on the parent issue #2888657: [meta] Less confusing and more consistent wording needed in module/theme add/install/update the Usability team is discussing the UI text for this and about 10 other issues. Until decisions are made, making another patch here would probably be premature.
Comment #126
jhodgdonRemoving Novice tag for now because we need to make a Usability team decision on how to proceed.