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

feat: added snapshot_list and single view UI #249

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sauraww
Copy link
Collaborator

@sauraww sauraww commented Sep 26, 2024

  • Added Snapshot UI List Page
  • Added Single Snapshot Page
    image
    image

@sauraww sauraww requested a review from a team as a code owner September 26, 2024 06:07
crates/frontend/src/api.rs Outdated Show resolved Hide resolved

<Route
ssr=SsrMode::Async
path="/admin/:tenant/snapshots"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we keep snapshots in the url or config-versions as we are using config-versions for the endpoint and the table as as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think in UI it would become confusing if endpoints are config-versions and page shows snapshot.

/>
<Route
ssr=SsrMode::Async
path="/admin/:tenant/snapshots/:version"
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here


view! {
<Suspense fallback=move || view! { <p>"Loading..."</p> }.into_view()>
{move || {
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 have some kind of heading here, taking inspiration from the experiment page maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah , I will add it.

@@ -314,3 +314,19 @@ pub struct FetchTypeTemplateResponse {
pub total_pages: i64,
pub data: Vec<TypeTemplate>,
}

#[derive(Debug, Clone, Deserialize, Serialize)]
pub struct SnapshotResponse {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You may have to change it similar to what i have done for the endpoint response type.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Use PaginatedResponse from superposition_types

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't have superposition_types in frontend , Let's move all the common types in a seperate PR.
What do you suggest?

@sauraww sauraww force-pushed the feat/snapshot-list-ui branch 4 times, most recently from 3e1cb92 to 7bf8df3 Compare September 26, 2024 11:24
@@ -51,6 +52,24 @@ pub async fn fetch_default_config(
Ok(response)
}

pub async fn get_snapshots(tenant: String) -> Result<SnapshotResponse, ServerFnError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pub async fn get_snapshots(tenant: String) -> Result<SnapshotResponse, ServerFnError> {
pub async fn fetch_snapshots(tenant: String) -> Result<SnapshotResponse, ServerFnError> {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Better to follow the same naming pattern across all functions

@@ -248,3 +267,24 @@ pub async fn fetch_types(
.await
.map_err(err_handler)
}

pub async fn get_config_by_version(
Copy link
Collaborator

Choose a reason for hiding this comment

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

An already existing function fetch_config can be re-purposed to fetch config by version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file can be named snapshot.rs

use leptos_router::use_params_map;

#[component]
pub fn SnapshotConfig() -> impl IntoView {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pub fn SnapshotConfig() -> impl IntoView {
pub fn snapshot() -> impl IntoView {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please run leptosfmt, indetation is off in view macro

);

view! {
<Suspense fallback=move || view! { <p>"Loading..."</p> }.into_view()>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use the loading skeleton for fallback in Suspense

use crate::api::get_snapshots;

#[derive(Serialize, Deserialize, Clone, Debug)]
struct CombinedResource {
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need of CombinedResource here, we just have one resource to load i.e. snapshots.

});

let table_columns =
create_memo(move |_| snapshot_table_columns(tenant_rs.get_untracked()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
create_memo(move |_| snapshot_table_columns(tenant_rs.get_untracked()));
create_memo(move |_| snapshot_table_columns(tenant_rs.get()));

this has to be reactive to any changes made to tenant.

let combined_resource: Resource<String, CombinedResource> = create_blocking_resource(
move || tenant_rs.get(),
|current_tenant| async move {
match get_snapshots(current_tenant.to_string()).await {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have a paginated API and still fetching all the snapshots at once. You can refer to experiment_list.rs to check how it can be done.

let href = format!("/admin/{}/snapshots/{}", tenant, id);
view! {
<div>
<A href=href class="btn-link">
Copy link
Collaborator

Choose a reason for hiding this comment

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

The hyperlink can be added directly to the id value, that should be enough.

@Datron
Copy link
Collaborator

Datron commented Sep 30, 2024

Target the main branch pleasse

@Datron Datron closed this Sep 30, 2024
@Datron Datron reopened this Sep 30, 2024
@mahatoankitkumar mahatoankitkumar force-pushed the feat/get-snapshots branch 3 times, most recently from f9d45c0 to 1a03a27 Compare September 30, 2024 11:35
Base automatically changed from feat/get-snapshots to main September 30, 2024 12:00
@sauraww sauraww force-pushed the feat/snapshot-list-ui branch 6 times, most recently from f81d64d to 46c1b9c Compare October 1, 2024 19:24
let client = reqwest::Client::new();
let host = use_host_server();

let url = format!("{host}/config-versions");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let url = format!("{host}/config-versions");
let url = format!("{host}/config/versions");

@@ -314,3 +314,19 @@ pub struct FetchTypeTemplateResponse {
pub total_pages: i64,
pub data: Vec<TypeTemplate>,
}

#[derive(Debug, Clone, Deserialize, Serialize)]
pub struct SnapshotResponse {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use PaginatedResponse from superposition_types

@sauraww sauraww force-pushed the feat/snapshot-list-ui branch 5 times, most recently from 3164cad to ffa2b37 Compare October 7, 2024 12:44
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.

4 participants