ui/toolbar

[xabolcs]

Fix for #183.

[xabolcs]

With commit d3e92771be7129f81fb41b5ce3a59b99204882ac the fully functional MemChaser is back! :+1: What do you think @whimboo?

I'm unable to continue without help!

• frame.css needs to be updated
• button icons need to be changed. They're now from chrome://browser/skin/menuPanel.png.

[whimboo]

With commit d3e9277 https://github.com/mozilla/memchaser/commit/d3e92771be7129f81fb41b5ce3a59b99204882ac the fully functional MemChaser is back! :+1: What do you think @whimboo https://github.com/whimboo?

Wohoo! I will have a look at it in a bit.

[xabolcs]

One more note .... ui/toolbar doesn't play well with the Developer Edition theme... The bad news is that widget worked fine with that dark theme.

[whimboo]

One more note .... |ui/toolbar| doesn't play well with the Developer Edition theme... The bad news is that |widget| worked fine with that dark theme.

Maybe a bug in the SDK which is not fixed yet?

[xabolcs]

@whimboo, next review please! :)

I think that all feature is back.

Except the tooltips. I found config.extension.widget_tooltips.logger_enabled and ..._disabled. Should I use them?

Would you mind to ask about "How to tooltip with ui/toolbar and ui/frame?" on IRC? OK, I did a workaround in the previous PR, but I don't like workarounds.

Before land all the old and unused code should be deleted. They are now uncommented. Except the tooltips.

[whimboo]

The logging button looks nice! Is the icon self-made or did you grab it from somewhere else and made modifications to it? For the menu popup button we could use something like this icon. Beside that can you please check if the icons can be made smaller? Compared to the others in the navbar they look large. Or is that the default and it cannot be changed?

For the panel can we use a styling which is similar to that one? That way we can workaround the problem with the darker dev edition.

[xabolcs]

The logging button looks nice! Is the icon self-made or did you grab it from somewhere else and made modifications to it?

The latter one. Link is in the svg.

For the menu popup button we could use something like this icon.

Thanks. What about the hamburger menu? Anyway, I'd like to include svg.

Beside that can you please check if the icons can be made smaller? Compared to the others in the navbar they look large. Or is that the default and it cannot be changed?

I have no control over the icon size. From ui/button documentation: ... Firefox will automatically choose the best-fit icon for your button ...

For the panel can we use a styling which is similar to that one?

If you reach the original author (of the image), then yes. :) I'm not good in CSS

That way we can workaround the problem with the darker dev edition.

Could it be covered by Issue #158?

[xabolcs]

Maybe a bug in the SDK which is not fixed yet?

Just filed it as Bug 1176194.

[whimboo]

Could it be covered by Issue #158?

Sure. Lets do it that way.

[xabolcs]

@whimboo, added a menu button image. I gave a try to the button you proposed, but it wasn't nice.

Are we done with this PR?

[whimboo]

I would suggest the following adjustments:

1. Update the color so it applies to the default icon colors as used in the toolbar buttons
2. If possible make the border thicker so it is similar to the history button icon. Maybe small rounded corners would fully polish it.

Otherwise I have seen that you can move the menu button to the popup menu and back to the main toolbar but not to the memchaser toolbar anymore. Is there a missing relationship? Also we might have to tweak the name in case the button is separately in the popup menu. Simply having 'Menu' doesn't let it relate to memchaser. Also any idea why I cannot customize the memchaser toolbar?

We could move all the comments of the last paragraph to a new issue if you want.

[xabolcs]

• Update the color so it applies to the default icon colors as used in the toolbar buttons

RGB codes are welcome. Firefox icons have a little shadow inside, that makes them darker, more visible.

• If possible make the border thicker so it is similar to the history button icon. Maybe small rounded corners would fully polish it.

I'll give it a try.

Otherwise I have seen that you can move the menu button to the popup menu and back to the main toolbar but not to the memchaser toolbar anymore. Is there a missing relationship?

Not sure about what you wrote. Could you provide screenshots, please?

Also we might have to tweak the name in case the button is separately in the popup menu. Simply having 'Menu' doesn't let it relate to memchaser.

Sure. E.g. "MemChaser Menu"?

Also any idea why I cannot customize the memchaser toolbar?

This is by design, ask some SDK guy! If I remember correctly,

• ui/frame can be attached only to ui/toolbar
• ui/toolbar elements can be ui/frame, ui/button/action, ui/button/toggle
• ui/toolbar can be customized only at creation time (through it's ctor), no real time customization

We could move all the comments of the last paragraph to a new issue if you want.

It would be nice to release a new version with this PR merged. We could decide at the end of week about the remained issues.

[whimboo]

RGB codes are welcome. Firefox icons have a little shadow inside, that makes them darker, more visible.

We don't need a shadow, just make it a bit lighter so it better fits.

Otherwise I have seen that you can move the menu button to the popup
menu and back to the main toolbar but not to the memchaser toolbar
anymore. Is there a missing relationship?

Not sure about what you wrote. Could you provide screenshots, please?

Right click the button and check the first context menu entry. Once you moved the button somewhere else you cannot move it back to our toolbar.

Also we might have to tweak the name in case the button is
separately in the popup menu. Simply having 'Menu' doesn't let it
relate to memchaser.

Sure. E.g. "MemChaser Menu"?

Memchaser Tools?

Also any idea why I cannot customize the memchaser toolbar?

This is by design, ask some SDK guy! If I remember correctly,

• |ui/frame| can be attached only to `ui/toolbar
• |ui/toolbar| elements can be |ui/frame|, |ui/button/action|, |ui/button/toggle|
• |ui/toolbar| can be customized only at creation time (through it's ctor), no real time customization

I feel this is a drastic limitation. Lets give it a feeling first and then I can maybe file a bug against the SDK.

We could move all the comments of the last paragraph to a new issue
if you want.

It would be nice to release a new version with this PR merged. We could decide at the end of week about the remained issues.

I think we should release at latest on Monday given that Tuesday will be my last day before I'm on PTO for a month.

[xabolcs]

We don't need a shadow, just make it a bit lighter so it better fits.

Thanks. Will do.

Right click the button and check the first context menu entry. Once you moved the button somewhere else you cannot move it back to our toolbar.

Uhh, ohh.... that should be an SDK bug. :)

Memchaser Tools?

Agreed.

I feel this is a drastic limitation. Lets give it a feeling first and then I can maybe file a bug against the SDK.

Feel free to file it. That's why I still prefer simple bootstrap addons.

I think we should release at latest on Monday given that Tuesday will be my last day before I'm on PTO for a month.

Agreed. :)

[whimboo]

Right click the button and check the first context menu entry. Once
you moved the button somewhere else you cannot move it back to our
toolbar.

Uhh, ohh.... that should be an SDK bug. :)

I will get one on file before I forget about it...

I feel this is a drastic limitation. Lets give it a feeling first
and then I can maybe file a bug against the SDK.

Feel free to file it. That's why I still prefer simple bootstrap addons.

Maybe you can explore how to do it next month? I would be happy to review a patch when I'm back. The current SDK looks (sorry) aweful.

[xabolcs]

Maybe you can explore how to do it next month? I would be happy to review a patch when I'm back. The current SDK looks (sorry) aweful.

Well ... about the UI I'd like to continue experimenting ExtSDK. ... About the non-UI part, I'd like to play with tests coverage, test/assert and Promise logger.

[xabolcs]

Don't forget to remove extension/data/widget/widget.css!

[xabolcs]

Ready to review again, up to commit 1ccda8da6b61f2d57e010a243042433665ff1766.

[whimboo]

That button looks fine. Lets remove all the old and obsolete code now so that I can do a proper review.

[xabolcs]

Lets remove all the old and obsolete code now so that I can do a proper review.

Sure! :+1:

[xabolcs]

Lets remove all the old and obsolete code now so that I can do a proper review.

Lets rewiew up to 2fa5e67044ebde6254903c8c90382994246a6249.

[whimboo]

I had a peak on the latest code and here some items we should update. Otherwise it looks great.

• I would put the icons into a separate subfolder, also please rename image for the menu button to menu_button.svg.
• The content of the tooltips should come from the config.js. Why aren't we showing this text at the moment? This applies to every element.
• Some lines (e.g. in garbage-collector.js) are too long. Please try to wrap parameters of functions

[xabolcs]

Thanks. I'll address your comments tonight.

The content of the tooltips should come from the config.js. Why aren't we showing this text at the moment? This applies to every element.

Say goodbye to tooltips for the elements in ui/frame for now. As I wrote earlier there is a workaround, but I won't do that again. This should work out of the box ... File a bug against Addon SDK, please!

The logging button could have the tooltip from config.js. That would work.

[whimboo]

Say goodbye to tooltips for the elements in |ui/frame| for now. As I wrote earlier there is a workaround, but I won't do that again. This should work out of the box ... File a bug against Addon SDK, please!

Hm, maybe you can do it? You have all the background for the underlying problem. I actually won't have to time to dig into that part until I leave for my PTO. We should add a comment to the bug in config.js then.

The logging button could have the tooltip from |config.js|. That would work.

And the menu button too I assume. :)

Thanks

[xabolcs]

Hm, maybe you can do it? You have all the background for the underlying problem. I actually won't have to time to dig into that part until I leave for my PTO. We should add a comment to the bug in config.js then.

There is no hurry about tooltips, IMHO. Sure, I can file a bug about that. For the release please write a notice about the absence of tooltips in the release notes!

And the menu button too I assume. :)

A new one? Where should I to store it in config.js?

[xabolcs]

I won't have enough time to address your comments tonight.

[whimboo]

I think all looks fine now. Can you please squash all the commits? If you cannot find an appropriate commit message I can land it later.

[xabolcs]

Thanks! Commit messages are always welcome.

[xabolcs]

Commits squashed.

[whimboo]

@xabolcs the file logging_disabled.svg has Windows line endings. Can you please fix it? When done you can use Replace widget module with new Addon SDK ui modules (#183) as commit message, and you can also push to master if you want.

[xabolcs]

All done.