mozillabrasil / sumo_live_helper

Helper Add-on for SUMO forum contributors
https://addons.mozilla.org/firefox/addon/sumo-live-helper-/
Mozilla Public License 2.0
7 stars 8 forks source link

Add sidebar functionality #86

Closed WesleyBranton closed 4 years ago

WesleyBranton commented 4 years ago

I'd like to add the option to allow users to open the question list in a sidebar instead of a browser popup. The implementation of this should be pretty painless, but it depends on issue #58 being completed first.

There may be some potential UI tweaks required for scaling, since the layout appears to be a fixed width and does not properly fit into a sidebar currently. I'm not sure if that will change when the redesign is complete. However, if it isn't, fixing the scaling could also fall under issue #74.

I'd should be fully able to implement this feature once the redesign is completed and it could be a good addition.

dannycolin commented 4 years ago

I'd like to add the option to allow users to open the question list in a sidebar instead of a browser popup.

I also wanted to implement a sidebar version instead of the popup :). For example, clicking on the browserAction icon could open a sidebar with the questions in it.

There may be some potential UI tweaks required for scaling, since the layout appears to be a fixed width

It should be resolved with the new design because I use Grid and Flexbox for it. It should also resolved the problem with #74

WesleyBranton commented 4 years ago

I did see that your new UI will fit quite nicely in a sidebar. I don't want to replace the popup option. I want to simply offer the option for users to chose. We should probably keep the standard browserAction popup for support on the mobile Firefox. An additional check may need to be performed to prevent mobile users from seeing the sidebar option in the preferences, since it's only available for desktop.

dannycolin commented 4 years ago

I want to simply offer the option for users to chose

Should we hide the list in the popup if the user choose to use the sidebar or we give both possibility at the same time. I'm asking in case it's the former, we'll need to add a new option in the preferences page.

WesleyBranton commented 4 years ago

No need to worry about adding options just yet. We can wait until the redesign is completed.

I envisioned it being implemented one of two ways:

  1. Pressing the browserAction would open the regular popup and there would be a sidebar button in there that would allow the user to open the sidebar.
  2. The user could set the popup to be completely replaced with the sidebar. Then pressing the browserAction would toggle the sidebar open or closed.

The only issue that I have with the first implementation is that opening the popup triggers an API call (multiple if the user is monitoring multiple products). So if the user needs to first open the popup to get to the sidebar that would trigger an API call when the popup opens and then another API call when the sidebar opens.

Really, it could be implemented either way. But your comment got me thinking that perhaps combining the two implementations could be the best option. Giving the user the option to set a default (sidebar or popup) via the preferences but then also providing a button to open the sidebar within the popup (if the popup is their default).

It's something we can think about. Doesn't need to be included in the current redesign code at the moment.

dannycolin commented 4 years ago

The only issue that I have with the first implementation is that opening the popup triggers an API call (multiple if the user is monitoring multiple products). So if the user needs to first open the popup to get to the sidebar that would trigger an API call when the popup opens and then another API call when the sidebar opens.

Would it be possible to separate the API call from the popup or sidebar. I mean, the questions could be fetched in the background and when you open the popup or sidebar, we only refresh the list from the locally stored data.

WesleyBranton commented 4 years ago

Would it be possible to separate the API call from the popup or sidebar. I mean, the questions could be fetched in the background and when you open the popup or sidebar, we only refresh the list from the locally stored data.

Yes, I'd imagine there would be a way that we could send something to the background.js file. My concern was more about the user opening the popup, triggering the API call and then going to the sidebar before the API call returns anything. Because then the UI would depend on the new API call made by the sidebar and the first API call would be sent to a dead popup window.

Although, I'd have to look at how the API calls are handled because it might not even be an issue. I might be able to make a minor tweak and get it to work properly.

Since I rewrote most of the API call code, I'd be fully willing and able to work on implementing the sidebar once the UI update is finished. I'd probably go for the merging both implementations together.

WesleyBranton commented 4 years ago

@dannycolin It's not urgent, but I need you to add a sidebar button into the font files that you have. I have a branch dedicated to the sidebar, so you can just add the new font file(s) there. Thanks!

WesleyBranton commented 4 years ago

There is a minor implementation issue with the sidebar. Apparently, Firefox doesn't believe that clicking the browserAction icon is a valid user input. Therefore, we are forced to go with implementation 1 and the user will be able to open both the sidebar and the popup. There won't be a toggle switch in the options UI.

dannycolin commented 4 years ago

@dannycolin It's not urgent, but I need you to add a sidebar button into the font files that you have. I have a branch dedicated to the sidebar, so you can just add the new font file(s) there. Thanks!

It was already on my todo list ;).

There is a minor implementation issue with the sidebar. Apparently, Firefox doesn't believe that clicking the browserAction icon is a valid user input.

This is strange. I've seen addons using a browserAction icon to open a page instead of a popup. I'll guess will go with implementation 1 like you said. However, I'm going to open a issue upstream for that. As a workaround, we could add a shortcut to open the sidebar. It'd, at least, save the user a few clicks.

WesleyBranton commented 4 years ago

We ran into a similar issue with the browserAction API when we first added notifications. The goal was to have the browserAction open when the user clicks on the notification, but the browserAction has the same restriction as the sidebarAction. Apparently, Firefox doesn't consider an onclick listener for the notifications API as a user input event either.

I think I will definitely add a keyboard shortcut, it's just a matter of deciding which one. I'm thinking maybe Ctrl + F1 or maybe just F1 because that's usually the shortcut for help. I just don't want to overlap an existing OS keyboard shortcut, which may be an issue with using F1.

dannycolin commented 4 years ago

I think I will definitely add a keyboard shortcut, it's just a matter of deciding which one. I'm thinking maybe Ctrl + F1 or maybe just F1 because that's usually the shortcut for help. I just don't want to overlap an existing OS keyboard shortcut, which may be an issue with using F1.

I, also, think we should avoid F keys with any modifier (ctrl, alt, shift). Ctrl + F1 is a bit hard to reach on some keyboard. I'll propose Ctrl + Shift + L. It isn't used by Firefox and the L is easier to remember/associate with the addons since the name is "Live Helper". Any thought?

WesleyBranton commented 4 years ago

Ctrl + Shift + L sounds good to me. I will still look into adding a toggle via the browserAction button.

jhonatasrm commented 4 years ago

Yes! Looks good to me too.. this week I'm a little bit off... But all these updates look great!

😃

WesleyBranton commented 4 years ago

I believe this is ready for implementation (see the branch to test), just waiting for the sidebar button to be added to the font by @dannycolin

dannycolin commented 4 years ago

I've pushed the new font to the test branch. You should be able to display the sidebar icon with the .pf-sidebar-left CSS class.