In the new module page we're collapsing the description and details for a module. It's a custom implementation and should be using the details element since that's exactly what it's made for.

The patch is pretty ugly, it's the quick and dirty version of what needs to be done for using details, it needs CSS cleanup, and proper use of the details form element.

Bonus, it removes a JS file since we're now using the same implementation as the rest of core for this. No idea why I haven't thought about it before.

Commit message:

Issue #1893072 by nod_, nagwani, Pete B: Use details element on module page.
CommentFileSizeAuthor
#21 Screen Shot 2013-07-02 at 21.37.57.png48.96 KBsaki007ster
#21 Screen Shot 2013-07-02 at 21.38.17.png43.13 KBsaki007ster
#21 Screen Shot 2013-07-02 at 21.38.37.png65.41 KBsaki007ster
#21 Screen Shot 2013-07-02 at 21.38.52.png68.11 KBsaki007ster
#21 Screen Shot 2013-07-02 at 21.51.18.png156.25 KBsaki007ster
#21 Screen Shot 2013-07-02 at 21.52.13.png152.95 KBsaki007ster
#20 core-module_page-details-1893072-20.patch6.05 KBPete B
#19 core-module_page-details-1893072-19.patch6.03 KBnod_
#16 core-module-page-details-1893072-16.patch6.83 KBPete B
#12 core-module-page-details-1893072-12.patch3.83 KBPete B
#3 module_page_details.png45.08 KBnod_
core-module_page-details.patch3.45 KBnod_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Issue tags: +Needs screenshots

How bad does it look?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Issue tags: +Usability

Also looping in the UX team.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
FileSize
module_page_details.png45.08 KB

Pretty bad :þ

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Issue tags: -Needs screenshots
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

LOL that is "awesome" :P

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

It's like a teenager putting on makeup for the first time. It's all over the place.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

It's like a teenager putting on makeup for the first time. It's all over the place.

HAHAHHAHAHAHAHA :D

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented

Oh, details. Anyways, following but consider it RTBC when it doesn't change anything visual (make sure you check all the wierdness around hover/selection).

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs review » Needs work

Reality check :)

droplet’s picture

droplet CreditAttribution: droplet commented

Bonus, it removes a JS file since we're now using the same implementation as the rest of core for this. No idea why I haven't thought about it before.

Only one desktop browser, the chrome supports it.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

we have a fallback, and a polyfill should be used by the time D8 ships #1839158: Replace collapse.js with a proper polyfill for <details>.

Pete B’s picture

Pete B CreditAttribution: Pete B commented
FileSize
core-module-page-details-1893072-12.patch3.83 KB

Patch to match the style of the original page using the new element.

Pete B’s picture

Pete B CreditAttribution: Pete B commented
Status: Needs work » Needs review

Set needs review

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs review » Needs work

Real nice, thanks!

Styling almost there, still a #system-modules .description { cursor: pointer } that should be removed. And the system.module.js file and the declaration removed from system_library_info() for this file.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

Closed #1898354: minimize module page's Description click area for this one. After the above comments are addressed this is RTBC for me.

( edit ) From the other issue nagwani should be credited for the patch too.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Issue summary: View changes

give comit message

Pete B’s picture

Pete B CreditAttribution: Pete B commented
FileSize
core-module-page-details-1893072-16.patch6.83 KB

Hi nod_,

Thanks a lot for the feedback. I have addresses the cursor pointer issue.

We actually still need system.modules.js because it is handling the search functionality, which needs to stay.

As an aside, I think we might need to start another issue to get test coverage for the module search, because patches #0 and #12 broke it and the test bot didn't notice?

I've fixed it now, and have removed the deprecated code from system.modules.js

Thanks,
Pete

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs work » Needs review

cool, i'll have a look. We can't test JS in core just yet. We rely on the fat module. So if we want to make sure it works, we need to write tests in that module.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

cool, i'll have a look. We can't test JS in core just yet. We rely on the fat module. So if we want to make sure it works, we need to write tests in that module.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
FileSize
core-module_page-details-1893072-19.patch6.03 KB

Made a couple of changes. took out a couple of CSS rules, minor change in the JS to avoid reselecting something we already selected, added an ID to the details element (otherwise we end up with <a> links with no href and i don't want that :p).

For the future, you don't need post a patch with commented CSS code, or JS or PHP actually. We can see in the diff what's taken out.

Pete B’s picture

Pete B CreditAttribution: Pete B commented
FileSize
core-module_page-details-1893072-20.patch6.05 KB

Reroll against latest core.

Thanks,
Pete

saki007ster’s picture

saki007ster
he/his
Location Hartford, Connecticut
CreditAttribution: saki007ster commented
FileSize
Screen Shot 2013-07-02 at 21.52.13.png152.95 KB
Screen Shot 2013-07-02 at 21.51.18.png156.25 KB
Screen Shot 2013-07-02 at 21.38.52.png68.11 KB
Screen Shot 2013-07-02 at 21.38.37.png65.41 KB
Screen Shot 2013-07-02 at 21.38.17.png43.13 KB
Screen Shot 2013-07-02 at 21.37.57.png48.96 KB

Tested this patch manually. Working fine for chrome,mozilla,safari,IE9 and mobile
Attaching screenshots for the reference.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

sounds like RTBC to me :)

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

name typo