fix: horizontal scroll for big conditions #207

ShubhranshuSanjeev · 2024-08-20T08:28:27Z

Problem

Conditions on Overrides and Resolve page overflowed the screen width forcing to horizontally scroll to check the content.

Solution Screenshots

Default view - content is truncated

Expanded view - on clicking the pills it will expanded to show the wrapped content.

ayushjain17

partial review

crates/frontend/src/pages/home.rs

crates/frontend/src/components/context_card.rs

crates/frontend/src/components/condition_pills.rs

ayushjain17 · 2024-08-21T09:49:19Z

crates/frontend/styles/tailwind.css

@@ -86,3 +87,12 @@
 .min-w-48 {


we can use tailwind config file to add such requirements

ayushjain17 · 2024-08-21T09:50:45Z

crates/frontend/styles/tailwind.css

@@ -27,6 +27,7 @@

 .context_condition{


can we not move these to tailwind itself ?

crates/frontend/src/pages/home.rs

ayushjain17 · 2024-08-21T09:55:16Z

crates/frontend/src/pages/home.rs

+                                class:text-gray-300={striked}
+                            >{key}</span>
+                        </td>
+                        <td class="min-w-48 max-w-72 font-mono" style="word-break: break-word;">


can we keep all styles in one single format, (preferably all in tailwind - break-words for this case)

I mostly just moved the code out of that big component function this just an old code when we were building the first cut for UI and never it touched it again, but sure I will make the change since we already made a class for that style.

crates/frontend/src/pages/home.rs

crates/frontend/src/components/condition_pills.rs

Datron · 2024-08-21T10:10:41Z

@ShubhranshuSanjeev few issues I saw in my testing

Control variants aren't fully displayed

It is not evident that the context pills can be clicked on. We should change the mouse pointer on hover
Single context elements don't need the And, the and sort of doesn't make sense since that is the default and only option.

I'm not sure about this redesign, arranging items horizontally isn't a safe bet for smaller screens, can we check this on a macbook air? I see a lot of white space wasted. I was hoping we would truncate the config key values as well until clicked on.

Datron · 2024-08-21T10:12:26Z

We made this full screen as well?

ShubhranshuSanjeev · 2024-08-21T10:15:51Z

I'm not sure about this redesign, arranging items horizontally isn't a safe bet for smaller screens, can we check this on a macbook air? I see a lot of white space wasted. I was hoping we would truncate the config key values as well until clicked on.

About the smaller screen, I have tested it on my side already it handles that, the table becomes horizontally scrollable, and there is no overflow of content.

ShubhranshuSanjeev · 2024-08-21T10:17:13Z

Single context elements don't need the And, the and sort of doesn't make sense since that is the default and only option.

Yes that is given its an and, but won't removing this will make UI bit inconsistent.

ShubhranshuSanjeev · 2024-08-21T10:18:00Z

It is not evident that the context pills can be clicked on. We should change the mouse pointer on hover

Sure, will change to pointer.

ShubhranshuSanjeev · 2024-08-21T10:18:59Z

Control variants aren't fully displayed

This works perfectly on my side, but let me check again.

ShubhranshuSanjeev · 2024-08-21T10:19:38Z

We made this full screen as well?

Didn't really change any styles for this page, but let me check

ShubhranshuSanjeev · 2024-08-21T10:49:42Z

I'm not sure about this redesign, arranging items horizontally isn't a safe bet for smaller screens, can we check this on a macbook air? I see a lot of white space wasted. I was hoping we would truncate the config key values as well until clicked on.

About the smaller screen, I have tested it on my side already it handles that, the table becomes horizontally scrollable, and there is no overflow of content.

Also for screens smaller than normal laptops the table and the conditions will be vertically arranged.

Datron · 2024-09-03T06:36:47Z

crates/frontend/src/hoc/layout.rs

@@ -36,7 +36,7 @@ pub fn Layout(children: Children) -> impl IntoView {
        <div>
            <SideNav resolved_path=path original_path=original_path.to_string()/>
            // params_map=params_map
-            <main class="ease-soft-in-out xl:ml-96 relative h-full max-h-screen rounded-xl transition-all duration-200 overflow-y-auto">
+            <main class="ease-soft-in-out xl:ml-[350px] relative h-full max-h-screen rounded-xl transition-all duration-200 overflow-y-auto">


Would rem and em instead of px be better?

Datron · 2024-09-03T06:37:32Z

crates/frontend/src/components/table.rs

+    #[prop(into, default = String::new())] cell_style: String,
+    #[prop(into, default = String::new())] style: String,
+    #[prop(into, default = String::new())] head_style: String,


Can we have some reasonable defaults or is empty string fine?

mahatoankitkumar

Remaining: Disabling actions on experiment's context.

ayushjain17

minor suggestions and few other changes

ayushjain17 · 2024-09-05T13:10:42Z

crates/frontend/src/components/context_card.rs

@@ -37,37 +45,41 @@ pub fn context_card(
    ];

    view! {
-        <div class="rounded-lg shadow bg-base-100 p-6 shadow">
+        <div class="rounded-lg shadow bg-base-100 p-6 shadow flex flex-col gap-4">


preferred order for classes:
"custom-non-tailwind classes"
"hidden"
"position z top/left/.."
"h w"
"m"
"p"
"flex props"
"text props"
"bg props"
"border props/rounded"
"shadow"
"transition/animation"

crates/frontend/src/components/condition_pills.rs

crates/frontend/src/components/table.rs

ayushjain17 · 2024-09-05T13:15:27Z

crates/frontend/src/components/table.rs

+                let TablePaginationProps { current_page, total_pages, on_prev, on_next, .. } = pagination_props
+                    .get_value();
+                view! {
+                    <div class="mt-2 flex justify-end">


can we pass these class styles to the Component itself?
redundant div, wrapping only one child

crates/frontend/src/pages/home.rs

ayushjain17 · 2024-09-05T13:17:48Z

crates/frontend/src/pages/home.rs

+                                class:text-gray-300={striked}
+                            >{key}</span>
+                        </td>
+                        <td class="min-w-48 max-w-72 font-mono" style="word-break: break-word;">


Suggested change

<td class="min-w-48 max-w-72 font-mono" style="word-break: break-word;">

<td class="min-w-48 max-w-72 font-mono break-words">

ayushjain17 · 2024-09-05T13:18:32Z

crates/frontend/src/pages/home.rs

+                                            id=context.id.clone()
+                                            class="xl:w-[400px] h-fit"
+                                        />
+                                        <div class="overflow-auto pt-5">


can we pass these props to the table itself ?

ayushjain17 · 2024-09-05T13:19:05Z

crates/frontend/src/pages/home.rs

+                        .collect::<Vec<_>>()
+                }
+            </ConditionCollapseProvider>
+            <div class="card bg-base-100 shadow m-6">


same, can we pass these props to the following div

ayushjain17 · 2024-09-05T13:19:43Z

crates/frontend/styles/tailwind.css

@@ -86,3 +87,12 @@
 .min-w-48 {


ayushjain17 · 2024-09-05T13:19:49Z

crates/frontend/styles/tailwind.css

@@ -27,6 +27,7 @@

 .context_condition{


changes requested have been addressed

ShubhranshuSanjeev marked this pull request as ready for review August 21, 2024 06:25

ShubhranshuSanjeev requested a review from a team as a code owner August 21, 2024 06:25

ShubhranshuSanjeev force-pushed the fix/horizontal-scroll branch 2 times, most recently from 1cd9ed7 to 552a695 Compare August 21, 2024 09:31

ayushjain17 requested changes Aug 21, 2024

View reviewed changes

ShubhranshuSanjeev force-pushed the fix/horizontal-scroll branch 3 times, most recently from 1834df8 to b24728b Compare August 22, 2024 13:52

Datron force-pushed the main branch 8 times, most recently from 6ca353c to 41073fc Compare September 2, 2024 10:08

Datron approved these changes Sep 3, 2024

View reviewed changes

ShubhranshuSanjeev force-pushed the fix/horizontal-scroll branch from b24728b to 6d0d6fa Compare September 4, 2024 17:27

Datron requested a review from ayushjain17 September 5, 2024 12:38

mahatoankitkumar approved these changes Sep 5, 2024

View reviewed changes

ayushjain17 previously requested changes Sep 5, 2024

View reviewed changes

fix: horizontal scroll for big conditions

5d1f8fe

ShubhranshuSanjeev and others added 3 commits September 5, 2024 20:42

fix: preveting multiple event handler register

8ce04ca

fix: removed horizontal arrangement of conditions and overrides

0b997c6

fix: resolved comments

6a32724

ShubhranshuSanjeev force-pushed the fix/horizontal-scroll branch from 3fde839 to 6a32724 Compare September 5, 2024 15:14

Datron approved these changes Sep 6, 2024

View reviewed changes

Datron merged commit f6519ef into main Sep 6, 2024
4 checks passed

Datron deleted the fix/horizontal-scroll branch September 6, 2024 05:45

ShubhranshuSanjeev linked an issue Sep 22, 2024 that may be closed by this pull request

Set condition pills width to fit content #109

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: horizontal scroll for big conditions #207

fix: horizontal scroll for big conditions #207

ShubhranshuSanjeev commented Aug 20, 2024 •

edited

Loading

ayushjain17 left a comment

ayushjain17 Aug 21, 2024

ayushjain17 Sep 5, 2024

ayushjain17 Aug 21, 2024

ayushjain17 Sep 5, 2024

ayushjain17 Aug 21, 2024

ShubhranshuSanjeev Aug 21, 2024

Datron commented Aug 21, 2024 •

edited

Loading

Datron commented Aug 21, 2024

ShubhranshuSanjeev commented Aug 21, 2024

ShubhranshuSanjeev commented Aug 21, 2024

ShubhranshuSanjeev commented Aug 21, 2024

ShubhranshuSanjeev commented Aug 21, 2024 •

edited

Loading

ShubhranshuSanjeev commented Aug 21, 2024

ShubhranshuSanjeev commented Aug 21, 2024 •

edited

Loading

Datron Sep 3, 2024

Datron Sep 3, 2024

mahatoankitkumar left a comment

ayushjain17 left a comment

ayushjain17 Sep 5, 2024

ayushjain17 Sep 5, 2024

ayushjain17 Sep 5, 2024

ayushjain17 Sep 5, 2024

ayushjain17 Sep 5, 2024

ayushjain17 Sep 5, 2024

ayushjain17 Sep 5, 2024

	<td class="min-w-48 max-w-72 font-mono" style="word-break: break-word;">
	<td class="min-w-48 max-w-72 font-mono break-words">

fix: horizontal scroll for big conditions #207

fix: horizontal scroll for big conditions #207

Conversation

ShubhranshuSanjeev commented Aug 20, 2024 • edited Loading

Problem

Solution Screenshots

ayushjain17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Datron commented Aug 21, 2024 • edited Loading

Datron commented Aug 21, 2024

ShubhranshuSanjeev commented Aug 21, 2024

ShubhranshuSanjeev commented Aug 21, 2024

ShubhranshuSanjeev commented Aug 21, 2024

ShubhranshuSanjeev commented Aug 21, 2024 • edited Loading

ShubhranshuSanjeev commented Aug 21, 2024

ShubhranshuSanjeev commented Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahatoankitkumar left a comment

Choose a reason for hiding this comment

ayushjain17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShubhranshuSanjeev commented Aug 20, 2024 •

edited

Loading

Datron commented Aug 21, 2024 •

edited

Loading

ShubhranshuSanjeev commented Aug 21, 2024 •

edited

Loading

ShubhranshuSanjeev commented Aug 21, 2024 •

edited

Loading