feat(deck-picker): draw tree hierarchy branch lines#21373
feat(deck-picker): draw tree hierarchy branch lines#21373ShaanNarendran wants to merge 1 commit into
Conversation
bc3f433 to
9694199
Compare
BrayanDSO
left a comment
There was a problem hiding this comment.
Screenshot tests would help
|
|
||
| /** | ||
| * Whether to highlight the selected deck. Usually true for fragmented (tablet) layouts | ||
| * where the deck contents are shown side-by-side, but false for phones. |
There was a problem hiding this comment.
the selected deck is highlighted in phones as well. What do you mean here?
There was a problem hiding this comment.
Whenever I highlighted a deck during testing, it stayed highlighted even after I finished tapping it. I could probably word the comment better, and put that in a separate commit. I'm not sure if that was an issue only on my local but thought I'd put a fix in here.
Do I just have to add screenshots of the page into the PR? |
|
A few suggestions:
|
We use the Roborazzi library for automated screenshot unit tests. Check the inheritors of |
I'm not sure if this is possible without making it too inefficient for large decks, will try this out.
I had a similar thought but I could never get it to look visually "uncluttered", can send some mocks once the rest of the code is perfected but I like what we have now.
@david-allison would love your thoughts here, do we go with lines connected to the chevron or not? |
f5c5df0 to
9a68a5f
Compare
This comment was marked as resolved.
This comment was marked as resolved.
9a68a5f to
6261296
Compare
I've tried a lot to avoid this but it's always come out wrong, may have to be a small quirk of the lines. It doesn't seem too noticeable to me, but ill still try and fix this while I finish the pr up tomorrow! |
|
I'm happy with the lines not connected to the chevron, but I feel the "dead zone" should be a few pixels smaller, in my screenshot it's hard to see the the horizontal line towards "blog" |
b5e70a0 to
005f886
Compare
@ShaanNarendran try this : draw vertical lines as two segments with a gap at the horizontal tip position, or use a single Path that outlines the entire corner shape without overlap |
| val childCount = parent.childCount | ||
| if (childCount == 0) return | ||
|
|
||
| val currentList = (parent.adapter as? DeckAdapter)?.currentList ?: return |
There was a problem hiding this comment.
MAJOR BLOCKER:
When DeckAdapter.submitList() is called while onDrawOver is executing, we may observe list in partially updated state which could lead to IndexOutOfBoundsException or can crash during scroll or adapter updates on large nested decks
It can be fix using RecyclerView.AdapterDataObserver or a callback from the adapter when the list changes
There was a problem hiding this comment.
I added null safety when we are looping so if there is an issue of a race condition it should just skip instead of crashing, would this work or should I go for the approach you mentioned?
005f886 to
77deec3
Compare
77deec3 to
e6e9411
Compare
Screen_recording_20260717_192949.webmJust an update on how it's going with incorporating all the feedback, I think it looks and feels much better now and I would love to get some more opinions |
6064cd5 to
de3823e
Compare
|
Snapshot diff report vs
All 4 changed screenshotsDeckPickerScreenshotTest
|
de3823e to
cb32a6d
Compare
cb32a6d to
a7c9a77
Compare
a7c9a77 to
14ce977
Compare
|
@ZornHadNoChoice Thank you so much for the in-depth review! I had most of this sitting as changes and I was just waiting for any other feedback to push it out, I think everything should be much cleaner now. The design is probably always going to be subjective, but I'm happy with this and hopefully it looks good. |
2a4fe9b to
472ea21
Compare
472ea21 to
4b8b16a
Compare






Note
Used Gemini 3.1 Pro to help write the code for the bit mask approach (horrible at math, so this was an experiment). All comments are my own, tested completely with the emulator and with a very large deck.
Purpose / Description
Draws hierarchy lines for the new deck picker redesign.
Fixes
Approach
We use a bit mask approach since looping through every row to draw all the lines is very inefficient, I've tried to comment it as much as possible with examples so that should explain the approach.
How Has This Been Tested?
Screen_recording_20260714_231431.webm
Checklist
Please, go through these checks before submitting the PR.