-
-
Notifications
You must be signed in to change notification settings - Fork 346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding collapse/expand JSON payloads feature. #1107
base: main
Are you sure you want to change the base?
Adding collapse/expand JSON payloads feature. #1107
Conversation
# Conflicts: # library/src/main/kotlin/com/chuckerteam/chucker/internal/ui/transaction/TransactionPayloadFragment.kt
Thanks for sending this over @jeancsanchez |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jeancsanchez thanks for your patience while I tested and reviewed this change. This feature is really promising and it would makes sense inside Chucker.
I do have a couple of points to share though.
- I'm unsure how this feature will play out with heavily nested objects, specifically because you won't split the lines anymore. An example is this simple response:
Before | After |
---|---|
And this example had only one level of nesting. Can we had some examples to the Chucker sample app to stress test this feature?
- Similarly, the feature was not working well big JSON, like the one used for the
/post
sample request. See
video-1697896725.mp4
We need to see what's going on here before we can merge this
@@ -53,6 +59,12 @@ internal class TransactionBodyAdapter : RecyclerView.Adapter<TransactionPayloadV | |||
TransactionPayloadViewHolder.BodyLineViewHolder(bodyItemBinding) | |||
} | |||
|
|||
TYPE_BODY_COLLAPSABLE -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TYPE_BODY_COLLAPSABLE -> { | |
TYPE_BODY_LINE_COLLAPSABLE -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename Collapsable
to LineCollapsable
everywhere?
android:tint="#F5F5F5" | ||
android:viewportWidth="24" | ||
android:viewportHeight="24"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
android:tint="#F5F5F5" | |
android:viewportWidth="24" | |
android:viewportHeight="24"> | |
android:tint="#FFFFFF" | |
android:viewportWidth="24.0" | |
android:viewportHeight="24.0"> |
android:tint="#F5F5F5" | ||
android:viewportWidth="960" | ||
android:viewportHeight="960"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
android:tint="#F5F5F5" | |
android:viewportWidth="960" | |
android:viewportHeight="960"> | |
android:tint="#FFFFFF" | |
android:viewportWidth="960" | |
android:viewportHeight="960"> |
Can you update the viewport side to be 24 and adapt the path instructions here?
mapToJsonElements() | ||
} catch (t: JsonSyntaxException) { | ||
Toast.makeText(context, t.message, Toast.LENGTH_LONG).show() | ||
Logger.error(t.message ?: "Error when formatting json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather display the "Error when formatting json"
in the Toast and dump the stacktrace in logcat instead
forEach { item -> | ||
when (item) { | ||
is TransactionPayloadItem.BodyLineItem -> bodyBuilder.append(item.line) | ||
is TransactionPayloadItem.HeaderItem, | ||
is TransactionPayloadItem.ImageItem -> newList.add(item) | ||
|
||
else -> Unit | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a super big fan of this approach where you re-add HeaderItem
and ImageItem
.
The only thing you need to do is to map BodyLineItem
to BodyCollapsibleLineItem
if the user clicked. Could you clean this up a bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also can you make this when exhaustive?
@@ -226,10 +240,182 @@ internal sealed class TransactionPayloadViewHolder(view: View) : RecyclerView.Vi | |||
const val LUMINANCE_THRESHOLD = 0.25 | |||
} | |||
} | |||
|
|||
internal class BodyJsonViewHolder( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we be consistent with the naming here?
This should be a BodyLineCollapsibleViewHolder
android:id="@+id/imgExpand" | ||
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:layout_marginTop="4dp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dimen/chucker_half_grid
android:layout_height="wrap_content" | ||
android:gravity="start" | ||
android:minLines="1" | ||
android:layout_marginTop="4dp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dimen/chucker_half_grid
private fun View.show() = apply { isVisible = true } | ||
private fun View.gone() = apply { isVisible = false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a ktx library inside androidx that was providing those functions already.
view.isGone = true
view.isVisible = true
Can we use it?
for ((key, value) in entrySet()) { | ||
JsonObject().also { | ||
it.add(key, value) | ||
attrList.add(TransactionPayloadItem.BodyCollapsableItem(jsonElement = it)) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you doing this? Could you explain?
📷 Screenshots
chucker-take-2.mov
chucker-take-1.mov
📄 Context
First of all, thank you so much for this awesome library!!!
I'm mobile developer, and I've been using this library in my job every single day. In some situations, I have to share my screen with a backend developer to show that the payload response has a bug and it should to be fixed on backend side.
However, it often becomes complicated to do so because the payload is too large and the search bar doesn't help much some times.
This change adds one more ViewHolder type where it is possible to collapse/expand JSON payloads, helping us to find some portion of the JSON more easily, specially when it has a lot of nested objects/arrays.
⏱️ Next steps
1 - Remove unnecessary commas when the payload is collapsed.
2 - Improve searching text when the payload is collapsed.