New Drawer module #252
New Drawer module #252
Conversation
|
This PR builds correctly, here is the generated apk.
You must be logged in for the link to work. This is an automatic comment created by a Github Action |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -165,7 +166,8 @@ | |||
|
|
|||
| // ------------------- initialize ------------------- | |||
|
|
|||
| private LinearLayout ll_mods; | |||
| private LinearLayout ll_shown_mods; | |||
| private LinearLayout ll_hidden_mods; | |||
Check warning
Code scanning / Pmd (reported by Codacy)
Fields should be declared at the top of the class, before any method declarations, constructors, initializers or inner classes.
| @@ -165,7 +166,8 @@ | |||
|
|
|||
| // ------------------- initialize ------------------- | |||
|
|
|||
| private LinearLayout ll_mods; | |||
| private LinearLayout ll_shown_mods; | |||
Check warning
Code scanning / Pmd (reported by Codacy)
Fields should be declared at the top of the class, before any method declarations, constructors, initializers or inner classes.
|
Looks good! Although I wonder if it could be possible with just one linear layout, maybe it was harder to manage the modules own visibility? Probably I played with it a bit and it behaves as expected, but I think it would need to detect when there are no visible modules on the drawer, to hide it (otherwise clicking the arrow does nothing). Another improvement would be to replace the arrow with something else (maybe two arrows?) It doesn't look bad, but it feels like it's missing something... I'll test some things, but feel free to try yourself too. Thanks! |
|
Didn't see it, but I guess two arrows would look weird. Maybe |
I like it, but from other PRs it seems like we are moving away from text in favor of icons.
What about this?
Or maybe using three horizontal dots (meatball, horizontal ellipsis)?
Yeah, I tried that. Because the modules are responsible of its own visibility they needed to know if they were in the drawer and the state of the drawer, it would also require tweaking each one to add extra logic in their respective onDisplayUrl(), aside from that each one would need to be notified when opening or closing the drawer.
I have a fix on my local branch now! |
Then is it really needed to change it? I personally like the current UI, a single arrow and nothing more. It looks clean. Although I guess in the end it's up to you. |
|
I prefer to use icons, yes. It's more of a personal decision, but I think it also has benefits related to translations (an icon is more 'universal', and even though the icon must have a text description, it's less important). I'm open to discussion though. Anyway, your second screenshot seems more cluttered. What about these? Ranked from top (my favorite) to bottom (less favorite)
|
|
Ohh, I like the first one! Both hands can easily reach the buttons, and it's still clean. As for the second one, I don't think it has any advantages over the current one. It's still in the middle and looks a bit worse, imo. The last one leaves a lot of free space in the middle, so it looks clean, that's for sure. But I guess it can be a bit hard to press if the border of the menu is too close to the end of the screen. |
|
On the three options (on all of them to be precise, Pablo's included) the full row is clickable, the icons placement is just visual. Even in that case, the first one does looks more "clean", I agree. |
Oh, I see. Even so, I think that placement is better, as a user will usually try to press the icon there, just like you press the button in other modules (like the Status module and a few others). Edit: "(on all of them to be precise, Pablo's included)" Oh 🤡 |
|
This PR builds correctly, here is the generated apk.
You must be logged in for the link to work. This is an automatic comment created by a Github Action |
|
I pushed the fix, no changes in the UI there.
This is why I wanted to put the border of the button, so the user clearly knows the whole module is a button and can comfortably press the button.
I also like the first one. Later I´m going to try something that I think it could work, if not, the best option is probably triangulo's first screenshot. |
|
The button is still the whole module, but it doesn't take the whole space visually so it doesn't feel cramped, I don't think many people will feel the need of tapping on the right/left of the module. The colors are muted so it doesn't stand out. Also is smaller in height.
|
|
I...don't like the full button border, I prefer it to be invisible. Or maybe tint all the background with a 25% or so transparency? I need to check that. What I really like is the horizontal line, in fact I tried to do it but didn't know how to do it properly, how did you make it?? For the muted colors, I have tried to make everything that is interactable colored, and everything that isn't grayscale (not everything follows this logic, I know, but most of it does and my idea is to change what doesn't). That button feels like it's disabled somehow, maybe it's just me. |
Well, I wouldn't say I did it properly, I was hoping to get it sorted out later if we went that way😅. The height of the separator is forced to 3px, and to keep it centered is just a bunch of space.
Agh, you are right, didn't think of it but it does give that feel indeed. |
|
Oh! That seems much easier, I though the line was inside the button somehow. In any case that may be even easier, I don't think you need any extra space views, just set the gravity to center and apply some padding. Btw, I won't be able to work on the app for a couple weeks, and I would like to review properly before merging, sorry. I just published version 2.12 to avoid taking more time, but on my return this is the first thing I'll check. If you want to develop other changes you can either create a new branch from master (in parallel, so the drawer code won't be there) or just create it from this one (in series, as if this one was master). In any case I can later merge/rebase as necessary, so don't worry about that and just do whichever you are more comfortable with. Great job in any case! and sorry again |
|
And the last tests I wanted to make. I prefer the first, but I don't dislike the second one.
|
No worries! I'll move on to the incognito issue then.
I prefer the second one. The first one... I just can't unsee it, to me it looks like an emoji |
|
I'm not a fan of the tinted background, so I prefer the first one. But I guess in the end both are fine.
It's been decided, we must go with the first one. |
|
This PR builds correctly, here is the generated apk.
You must be logged in for the link to work. This is an automatic comment created by a Github Action |
| /** | ||
| * Last call for any update a module may need (like the drawer module needing to know how many modules are visible). | ||
| */ | ||
| public void onFinishUrl(UrlData urlData) { |
Check warning
Code scanning / Pmd (reported by Codacy)
Document empty method body
| @@ -165,7 +178,8 @@ | |||
|
|
|||
| // ------------------- initialize ------------------- | |||
|
|
|||
| private LinearLayout ll_mods; | |||
| private LinearLayout ll_main; | |||
| private LinearLayout ll_drawer; | |||
Check warning
Code scanning / Pmd (reported by Codacy)
Fields should be declared at the top of the class, before any method declarations, constructors, initializers or inner classes.
| @@ -165,7 +178,8 @@ | |||
|
|
|||
| // ------------------- initialize ------------------- | |||
|
|
|||
| private LinearLayout ll_mods; | |||
| private LinearLayout ll_main; | |||
Check warning
Code scanning / Pmd (reported by Codacy)
Fields should be declared at the top of the class, before any method declarations, constructors, initializers or inner classes.
|
Ok, so... Hi! I'm back. Let me be honest, I don't like how the drawer module requires 'special' features, maybe it should have been better to add it as a built-in feature, I don't know. I still like it being a module though. I tried to extract the logic, move it to a different file, even using callbacks; but everything was just worse because your solution is both simple and easy. So, I just made some minor renames and cleanups, and made a special callback for the visibility problem (I don't like the modules to be ordered, they were in the past and it had a lot of issues) and kept your logic. I'll test this version on my device for a couple days, if I don't see anything strange I'll merge it! |
I feel the same but it required minimal changes. At the very least it needs a way to be visualized in the modules screen, that would mean either add it as a module or make changes to the module screen, which, among other things, I felt it would take more time.
I did two versions of this, as I wasn't conviced either on sorting the modules, but the other one, while minimal, required changes in all modules, as I wanted to reuse the module ids, but that is currently not possible from the Btw, I'm still working on the incognito issue, but don't expect it soon as it is taking me some time. |
|
After some daily usage, it is stable and working. Merged! 🥳 |
Solves #224.
Not much to explain really, modules that can be hidden are in a separate layout that can change its visibility. The module itself has a button to toggle the visibility.