Skip to content
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

fix: using HtmlDisplay trait to show json in html #245

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ShubhranshuSanjeev
Copy link
Collaborator

@ShubhranshuSanjeev ShubhranshuSanjeev commented Sep 22, 2024

Description

  • Fixes display of JSON values across the frontend.
  • Uses html_display to render json values
  • Removes trim from current frontend code

@ShubhranshuSanjeev ShubhranshuSanjeev linked an issue Sep 22, 2024 that may be closed by this pull request
@ShubhranshuSanjeev ShubhranshuSanjeev force-pushed the fix/override-display branch 2 times, most recently from fa0274a to 2405022 Compare September 23, 2024 12:39
@ShubhranshuSanjeev ShubhranshuSanjeev marked this pull request as ready for review September 23, 2024 14:00
@ShubhranshuSanjeev ShubhranshuSanjeev requested a review from a team as a code owner September 23, 2024 14:00
Copy link
Collaborator

@Datron Datron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Write a PR description, attach images of the change

Comment on lines +34 to +37
Map::from_iter(vec![
(String::from("KEY"), Value::String(k.clone())),
(String::from("VALUE"), v),
])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this tested? Or can we remove this because of @sauraww 's change earlier?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is tested, I am just getting rid of trim logic, as html_display will take care of showing these values without quotes in frontend.

Copy link
Collaborator

@ayushjain17 ayushjain17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not complete review

row.get(cname).unwrap_or(&Value::String("".to_string())),
);
let value: String =
row.get(cname).unwrap_or(&Value::String(String::new())).html_display();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what difference is html_display creating here ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is similar to generate_table_row_str, just using html_display everywhere for uniformity.

}
view
}}

</div>
</div>
</div> <div class="card bg-base-100 max-w-screen shadow m-5">
</div>
<div class="card bg-base-100 max-w-screen shadow m-5">
<div class="card-body">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meanwhile can we also remove this redundant div

}
view
}}

</div>
</div>
</div> <div class="card bg-base-100 max-w-screen shadow m-5">
</div>
<div class="card bg-base-100 max-w-screen shadow m-5">
<div class="card-body">
<h2 class="card-title">Variants</h2>
<div class="overflow-x-auto overflow-y-auto">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this as well seems redundant

for value in values.iter() {
if value.is_object() && value.get("var").is_some() {
continue;
}

let value_str = match value {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this not be replaced by html_display ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, will do it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change auto-focuses on the dropdown input element whenever it opens up.

row.get(cname).unwrap_or(&Value::String("".to_string())),
);
let value: String =
row.get(cname).unwrap_or(&Value::String(String::new())).html_display();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is similar to generate_table_row_str, just using html_display everywhere for uniformity.

let experiment = store_value(experiment);
let contexts = experiment.with_value(|v| v.context.clone());
let badge_class = match experiment.with_value(|v| v.status) {
ExperimentStatusType::CREATED => "badge text-white ml-3 mb-1 badge-xl badge-info",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we avoid repetition over here, the class changing here seems to be badge-info with badge-warning and badge-success

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

<label
tabindex="0"
class=format!("{} {}", class, btn_class)
on:click:undelegated=move |_| {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, how was this supposed to work ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't get your question

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was asking how was this change helping in solving the autofocus issue, couldn't understand the logic by looking at the code

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of implementation, I have an input element ref, every click on the button, I am using the input element ref to focus on the input box.

Can you specify what exactly you didn't understand?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To further add on this, the component library we are using keeps the dropdown mounted on the DOM, so autofocus do focus on the input box but since we make other interactions that focus is lost.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other way of doing this can be taking over the logic to display dropdown using signals, but I don't want to manually manage close for any click outside of dropdown.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keeps the dropdown mounted on the DOM

why is it always mounted ?
shouldn't we just unmount it when not in use
anyways, the behaviour which we simply wanted is the moment it becomes visible (gets mounted in ideal case), the input component should get focused

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we just unmount it when not in use

Ideally yes.

why is it always mounted ?

That's how daisy UI does it.
It just changes the visibility to hidden and opacity to 0.

@@ -86,7 +97,8 @@ where
type="text"
class="grow"
placeholder="Search"
name=name
ref_=node_ref
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wont this suffice ?

Suggested change
ref_=node_ref
autofocus={true}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That wasn't working, so went ahead with this approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Presentation of keys needs to be standardized in UI
3 participants