Support from Acquia helps fund testing for Drupal Acquia logo

Comments

radimklaska’s picture

Assigned: Unassigned » radimklaska
radimklaska’s picture

Assigned: radimklaska » Unassigned
Status: Active » Needs review
FileSize
1.39 KB

Doc in this module is pretty straight forward.

Link to the d.o documentation was linking D7 version. So I created D8 "clone" with updated screenshots and menu item names here: https://drupal.org/node/2099449

Attaching patch with updated URL.

jhodgdon’s picture

Status: Needs review » Needs work

Actually, we do want the link to go to drupal.org/documentation/modules/overlay. The page will may to be updated for Drupal 8, but we do want that to be the link.

See
https://drupal.org/node/632280#url-note
for URL format though.

BarisW’s picture

So does this mean that this issue can be closed then?

radimklaska’s picture

Thanks for your feedback. So right now the patch is not needed because there already is correct URL in the code.

Should I do something about the new doc page I created, or should I just wait until someone redirects the "nice" URL?

lostkangaroo’s picture

The patch is still needed but the string tokens use ! rather than @

radimklaska’s picture

Assigned: Unassigned » radimklaska
radimklaska’s picture

Assigned: radimklaska » Unassigned
Status: Needs work » Needs review
FileSize
1.41 KB

Attached patch changes just the token mentioned in #6 from @ to !

jhodgdon’s picture

Status: Needs review » Needs work

The drupal.org link also needs to be https. Thanks!

And sorry for taking so long to get this patch reviewed... I am behind from all the great work done at Prague!

radimklaska’s picture

Np, thanks for review. :-) New patch attached...

radimklaska’s picture

Status: Needs work » Needs review
ifrik’s picture

Status: Needs review » Needs work

Thanks @radimklaska, the links are fine like that.

Now the text needs reviewing as well.

ifrik’s picture

I tried to make the About section easy to read, and added a use: how to turn off the overlay per user.

ifrik’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Thanks! I am not sure I prefer the new wording completely:

The Overlay module uses JavaScript to display administration pages as as an overlay on your current page, instead of replacing the page in your browser window. This ensures that you return to that page once you close the overlay, even if you have visited several administration pages.

In the first sentence, I find "the page" to be a bit ambiguous, which also makes "that page" a bit ambiguous in the next sentence.

The previous wording that this patch is replacing:

The Overlay module makes the administration pages on your site display in a JavaScript overlay of the page you were viewing when you clicked the administrative link, instead of replacing the page in your browser window.

I think the previous wording for the first sentence is actually clearer, even though the second is shorter... Maybe you could keep that sentence and then replace "that page" with "the page you were viewing" in the second sentence?

The new Uses item is great!

ifrik’s picture

Status: Needs work » Needs review
FileSize
4.65 KB
1.7 KB

I see what you mean. My main issue was trying to get change the first part of the sentence because I kept on reading "site display" as a noun, instead of as "makes .... display".
How about?

The Overlay module uses JavaScript to open administration pages in an overlay of the page you were viewing when you clicked the administrative link, instead of replacing the page in you browser window. This ensures that you return to the page you were viewing once you close the overlay, even if you have visited several administration pages.

lostkangaroo’s picture

ifrik the last interdiff looks like it picked up some extra work you were doing with the text module, but tis a silly thing.

The changed wording is easy to understand and to the point which makes it effective in my book. If jhodgdon approves as well I will do a manual review this afternoon.

jhodgdon’s picture

Status: Needs review » Needs work

RE #17 - the interdiff did, but the patch didn't. :)

The patch has a typo: "... instead of replacing the page in **you** browser window" --> your

Other than that, I think this is fine... Although do we ever call a user account page a "Profile" page in the UI? If not, maybe we should say something like "by editing their user account settings" or something like that in the Uses bullet?

ifrik’s picture

Typo fixed.
In D8, when you click on your user name, you get a menu with "view profile" and "edit profile", so I suppose we should use "Profile".

ifrik’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Sounds good, thanks for the clarification! I think this one is good to go.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks everyone! Committed to 8.x.

Status: Fixed » Closed (fixed)

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