search

ByzantineFailure / BPM-for-Discord

BPM for Discord's Desktop App. Includes one-click installers, update notifications, and custom script support.
GNU Affero General Public License v3.0
17 stars 8 forks source link

BPM Button is in an Inconvenient Location #55

Closed AmaziaTheAmazing closed 8 years ago

AmaziaTheAmazing commented 8 years ago

The BPM button is in the bottom left while all other Discord related buttons are in the top right (near the pinned messages and user list button). This both takes up screen space that could be used for the sever list otherwise, and leaves space in the top right unused. I'm not necessarily sure about a unilateral change, but a setting would definitely be appreciated.

ByzantineFailure commented 8 years ago

@AmaziaTheAmazing Let me play around with it and I'll see what I can do.

ByzantineFailure commented 8 years ago

This is actually going to be nontrivial -- the entire header changes when different servers/friends are selected. Additionally, the name and classes of the one thing that doesn't change (the window buttons) are potentially platform-specific. I'm going to take some time to look into this, but I may try and see if there's a better place to put this (e.g. the chatbox).

ByzantineFailure commented 8 years ago

Clarification: The entire top header changes when the "Friends" icon is selected. In fact, the entire application wrap changes.

Luckily, the header's class remains the same. So, ideally speaking, we can use a MutationObserver on the old header node to detect when it's removed, and just snag and place the new button onto the page after React has finished playing around with the DOM (note: They both have the same header div class, header-toolbar).

Side note, it's super important we retain the same DOM node as we rely on code from BPM's core to attach the listener on the search button. The Discord code which creates and attaches this has no knowledge of the function which creates the listener and actually setting up the code to create/attach this listener can/will be pretty painful/annoying/spaghetti.

I don't particularly like this, as it feels quite brittle as compared with the fairly-safe method currently used to inject the button, but I'll play around with it and see if I can come up with something reasonable.

ByzantineFailure commented 8 years ago

I think I have something up and cooking, but there's an issue where SweetieBelle's position isn't always consistent (she's not always on the end) which seems to be the result of a race condition between my MutationObserver and React adding the elements. Either way, pushing current progress to a branch.

ByzantineFailure commented 8 years ago

56 opened to keep track of progress on this issue.

ByzantineFailure commented 8 years ago

I think I have this worked out -- I'm going to bake test this on my machine and NOT merge it into the mainline until at least a week of me using this and it not breaking has passed.

ByzantineFailure commented 8 years ago

Additionally I'm going to poll around to see if this is desired behavior or not -- I'm not really willing to make this a setting as the code changes are pretty... drastic. Well, maybe I am, I dunno, it depends upon how lazy I feel.

ByzantineFailure commented 8 years ago

I implemented a setting. This is 100% going in once I'm sure it's stable.

ByzantineFailure commented 8 years ago

Code merged in, this will go out next release.