If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Make generic notification element

VERIFIED FIXED in 5.0.6

Status

addons.mozilla.org Graveyard
Public Pages
VERIFIED FIXED
9 years ago
2 years ago

People

(Reporter: wenzel, Assigned: wenzel)

Tracking

unspecified
5.0.6
Dependency tree / graph

Details

Attachments

(2 attachments)

(Assignee)

Description

9 years ago
We need a generic notification element to be shown on top of any pages where we have something important to say (and should use it on the collections pages etc.)

I have some code on the way for this. It'll need to be skinned "clearlefty" later, but that's out of scope here.
(Assignee)

Comment 1

9 years ago
Created attachment 377063 [details] [diff] [review]
Patch, rev. 1

This is a generic notification element, not skinned very much yet (thus the empty rules in the CSS file), but it works, and this patch uses it on the collections pages already.
Attachment #377063 - Flags: review?(jbalogh)
The site-notice code already exists.  Can we either use that or make that use this as well?
(Assignee)

Comment 3

9 years ago
(In reply to comment #2)
> The site-notice code already exists.  Can we either use that or make that use
> this as well?

Do you mind sharing where this code is?
Sure.  It's in header.thtml and, more recently, amo2009.thtml.  I'm just talking about this and the related css:

    <?php if (is_object($this->controller->Config) && $this->controller->Config->getValue('site_notice') != ''): ?>
        <div id="site-notice"><p><?= $this->controller->Config->getValue('site_notice') ?></p></div>
    <?php endif ?>
(Assignee)

Comment 5

9 years ago
Thanks, I was poking around the header files but I must have overlooked it.

I can make it use the notification element, but I am not sure if these two are similar enough? The site notice adds a full-width line to the very top of the screen (similar to Firefox's notifications), while this patch adds a box to the content part of the screen. I don't think we want either to look like the other. Do you disagree?
I didn't actually apply your patch and look at it, I just thought if we had a generic element we could use it for both.  Since you've seen them both it's your call.
(Assignee)

Comment 7

9 years ago
All right, after looking at both, I must say they are different enough that making them use the same element does not seem a good idea to me. Thanks for the suggestion though!
I agree with Fred; they're pretty different.
(Assignee)

Comment 9

9 years ago
Of course, what I did not do in this patch is add this as a global notification framework (as in: you can just call notify(...) in the controller and it'll magically show up on the next rendered view). Is that what we want instead?

Due to our bandwagon/redesign time constraint, what I have here may be sufficient for now, but it seems to me that a more general solution could allow us to get rid of all the flash() ugliness we have on the site.
I think some of the flash() messages should continue to be full page and jarring (add-on not found) but some could be replaced (adding a user to a group).  

I was looking at this the other day and if we make a flash.thtml it will automatically use that but inside of an entire page our "Add-on not found!" messages look pretty weak sauce.
(Assignee)

Comment 11

9 years ago
That's obviously out of scope here, but maybe if we made a flash.thtml that looks a little more like it belongs to the page, it'd start being only half as obnoxious ;)

Updated

9 years ago
Attachment #377063 - Flags: review?(jbalogh) → review+
Comment on attachment 377063 [details] [diff] [review]
Patch, rev. 1

This looks sweet!  I like what you've done with delete collection, I hadn't seen that yet.


>diff --git a/site/app/views/collections/view.thtml b/site/app/views/collections/view.thtml
>+    if (!empty($collection_created)) {
>+        echo $this->renderElement('notification', array(
>+            'msg' => 'Your collection is now ready!',
>+            'description' => '<p>You can view your new collection below. If you\'d '
>+                .'like to set a collection nickname, upload an icon, or change '
>+                .'additional settings, please visit the '
>+                .$html->link('Manage Collections', "/collections/edit/{$collection['Collection']['uuid']}")
>+                .' page.</p>'
>+                .'<p>Your collection can be accessed at this location: '
>+                .$html->link(SITE_URL.$html->url("/collection/{$collection['Collection']['uuid']}"), "/collection/{$collection['Collection']['uuid']}")

Could you put the collection url in a variable to improve readability?

>+.notification-box.notification {}
>+.notification-box.error {}

That seems odd.  Why do you need it?
(Assignee)

Updated

9 years ago
Blocks: 493007
(Assignee)

Comment 13

9 years ago
(In reply to comment #12)
> (From update of attachment 377063 [details] [diff] [review])
> This looks sweet!  I like what you've done with delete collection, I hadn't
> seen that yet.
> 
> 
> >diff --git a/site/app/views/collections/view.thtml b/site/app/views/collections/view.thtml
> >+    if (!empty($collection_created)) {
> >+        echo $this->renderElement('notification', array(
> >+            'msg' => 'Your collection is now ready!',
> >+            'description' => '<p>You can view your new collection below. If you\'d '
> >+                .'like to set a collection nickname, upload an icon, or change '
> >+                .'additional settings, please visit the '
> >+                .$html->link('Manage Collections', "/collections/edit/{$collection['Collection']['uuid']}")
> >+                .' page.</p>'
> >+                .'<p>Your collection can be accessed at this location: '
> >+                .$html->link(SITE_URL.$html->url("/collection/{$collection['Collection']['uuid']}"), "/collection/{$collection['Collection']['uuid']}")
> 
> Could you put the collection url in a variable to improve readability?

No, this stuff will change slightly in bug 491794, though.

> 
> >+.notification-box.notification {}
> >+.notification-box.error {}
> 
> That seems odd.  Why do you need it?

See comment 1. I removed it though, however I still believe these should be skinned... at all. I filed bug 493007.

r25713.
(Assignee)

Comment 14

9 years ago
QA: When you edit a collection, then save it, there should be a banner-like notification telling you you just saved it correctly. If that shows up, this element works fine.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: push-needed
Resolution: --- → FIXED
Target Milestone: --- → 5.0.6
Created attachment 377584 [details]
Screenshot of notification box
Verified FIXED; see screenshot, above, in comment 15.
Status: RESOLVED → VERIFIED
(Assignee)

Updated

9 years ago
Blocks: 493986
removing "push-needed" from 105 AMO 5.0.6 bugs; filter on "I hate stephend!"
Keywords: push-needed
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.