Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lostkangaroo’s picture

Component: documentation » breakpoint.module
Issue tags: +Help text
FileSize
3.77 KB

Tokenized the links and added a link to the DO Handbook Page.

lostkangaroo’s picture

Status: Active » Needs review
batigolix’s picture

I changed the reference to the handbook to be more conform the standard example (https://drupal.org/node/632280)

And changed the tokens from @something to !something, because I think that is the standard now (I'm a bit confused about this)

I also doubt about the points in the Uses section. Normally we would list action for the site builder here, but the breakpoint has no UI. So not sure if we should list all these options here ...

lostkangaroo’s picture

I had some worries about the breakpoint UI also since the handbook page lists a possible module in the works. Thanks for fixing the @'s as someone pointed out to me recently in IRC that even remote links should be fine to not be passed through checkplain as long as they are in code.

Will mark RTBC as soon as it passes testing. Good Job!

batigolix’s picture

I would like some more feedback from others (especially about the Uses section) before marking this RTBC.

Status: Needs review » Needs work

The last submitted patch, drupal8.breakpoint-module.2091297-3.patch, failed testing.

lostkangaroo’s picture

Status: Needs work » Needs review

I don't think it will be necessary to wait honestly based on the discussions about hook_help text when it originally went in #1734642-158: Move breakpoint module into core. In this case we really only need to update the links.

lostkangaroo’s picture

jhodgdon’s picture

Status: Needs review » Needs work

Thanks!

Could we make the formatting here more uniform in Uses? One of the Uses items uses DT/DD/P, and the other uses DT/DD/DD. Let's just use multiple DD tags instead of P tags?

I also think that the Wikipedia link should not be taken out of the t() string. If you'll notice, there is an "en" in the URL, meaning it's going to an English page. If you leave the link in, then translators can switch that to a link appropriate for their language.

batigolix’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
3.03 KB
2.49 KB

The formatting seems pretty standard to me: http://jsfiddle.net/mLghm/
There is just one DL that contains two paragraphs P

The patch fixes the Wikipedia link.

jhodgdon’s picture

Issue tags: +Needs manual testing

Point taken. I think this is ready! It needs a quick manual test:
- Make sure the links all work.
- Make sure the formatting is OK.
Normally I would say we would need to make sure that all UI text matches what you see in the UI, but this module doesn't have a UI.

jhodgdon’s picture

Come to think of it... Do you think we should mention explicitly in the help that the module has no UI? Other modules that do not have UI say something about that in their help.

batigolix’s picture

It doesnt hurt to mention it.

There is a UI in contrib, though.

We have a similar situation with the tour module. No UI a contrib module exists that lets you create tours via the UI.

I am not sure if it is a good idea to explicitly link from hook_help to the contrib modules project page in such cases, as we have no idea if these modules will be usable when D8 launches (although there is plenty of time left ;). Or if a much better contrib module is available that we do not know of yet.

The help text of the taxonomy module (typically an area with plenty of contrib activity) mentions:
There are <a href="@taxcontrib">many contributed modules</a> that extend the behavior of the Taxonomy module for both display and organization of terms.

Maybe something similar could be added in this help text?

Other remaining tasks:
- Make sure the links all work.
- Make sure the formatting is OK. (as mentioned in #11)

jhodgdon’s picture

Regarding contrib modules, we did suggest particular contrib modules in some of the D7 help pages too, though I cannot remember exactly what... maybe Token? So I think that is fine, especially if you say "Contributed modules such as (link)Breakpoint UI(end link) may provide a user interface." (In this case, linking to that particular module sounds like a good idea, but leaving it open that there might be others is also a good idea).

batigolix’s picture

Just adding a note that the help should make clear that breakpoints are defined in the theme configuration

jhodgdon’s picture

Status: Needs review » Needs work

status as per last few comments

batigolix’s picture

Status: Needs work » Needs review
FileSize
3.79 KB
3.5 KB

This patch adds the link to the contrib module and explicitly mentions configuration.

I am still not really convinced about this help text.

Here we have the situation of a new core module with a maintainer that actually made an effort to write a hook_help text. In contrast to many other new modules where we start with nothing.

This takes us back to the pre-D7 times where some help text were well and others were minimally written. There was little consistency.

Since D7 we have much more consistent help text and I feel the text in the current patch does not fit in very well with the rest. The Uses section contains points that are not actually possible actions for site administrators. They are theory about Breakpoints.

I feel all of the current proposal text could be compressed into one (long) About section.

jhodgdon’s picture

Status: Needs review » Needs work

I agree with the assessment in #17. We normally reserve the "Uses" section for things a user can do in the UI, and this module has no UI.

How about this for a proposed re-organization? I think all the info there is great and we just need to present it in a manner more like our standard:

- About
(include the paragraph from the current patch)
-- h4 Terminology
DT Breakpoint
DD (define what a breakpoint is)
DT Media query
DD (definition)
DT Resolution multiplier
DD (definition)
DT Breakpoint group
DD (definition

- Uses
DT Defining breakpoints and breakpoint groups
DD Modules and themes can use the API provided by the Breakpoint module to define breakpoints and breakpoint groups, and to assign resolution multipliers to breakpoints. For more information, see https://drupal.org/node/1803874

[Note: I hope this notation is clear... I'm suggesting that we have an H4 header within About called "Terminology", and that it contain a DL list of terms, and that we have only one item in Uses.]

batigolix’s picture

That is a good idea.

In this patch I restructured the text according to the outline in #18

I did not (have/take) time to review properly.

One other question remains:
The More info link you suggested for the Uses section was already in the About section. We want 2 x the same link in this help?

jhodgdon’s picture

Status: Needs review » Needs work

I think you're right that the more information link only needs to be in one place. I didn't realize that was the same page as documentation/modules/breakpoint probably, but you're right, it is. I think that the new format reads well, and I didn't see any typos or grammatical problems.

So other than removing the duplicate link, I think this is ready to go, aside from a manual test that would:
- Verify that all the links work
- Verify that the formatting is OK.

lostkangaroo’s picture

Was just about to do the manual and found a whitespace error in the patch at the end of Line 30.

batigolix’s picture

Status: Needs work » Needs review
FileSize
4.18 KB
5.12 KB

There we go.

- removed trailing white space
- removed 2nd link to docs
- corrected name of module (was "Breakpoints")

jhodgdon’s picture

Thanks! Time for a manual test then.

robcarr’s picture

Status: Needs review » Reviewed & tested by the community

Text seems good and useful links to more detailed documentation. Changing to RTBC.

batigolix’s picture

Status: Reviewed & tested by the community » Needs review

This needs a manual test first:

- Verify that all the links work
- Verify that all mentions of pages/text within the UI match what is seen in the UI
- Verify that the formatting is OK.

batigolix’s picture

Issue tags: +Novice
jhodgdon’s picture

Component: breakpoint.module » documentation
Status: Needs review » Reviewed & tested by the community

I gave this a manual review today and it all looks good!

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks again everyone! Committed to 8.x.

Status: Fixed » Closed (fixed)

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