main UI - directives and table views #168

jonathanbataire · 2024-05-11T17:05:18Z

Description

This PR is 2/3 of #172 and #157
Fixes #131

jonathanbataire · 2024-05-11T17:25:02Z

src/liquid/app/nav.html

renamed to header.html to distinguish between header and nav bar

jonathanbataire · 2024-05-11T17:26:58Z

src/routes/app.ts

apply changes and filter modified to filter by contactType

freddieptf · 2024-05-16T17:23:42Z

src/scss/_header.scss

+$dropdown-width: 200px;
+
+.navbar {
+    background-color: $_medic-grey;


I think we should keep the navbar white and remove the text next to the logo. The previous look with the logo only was clean

freddieptf · 2024-05-16T18:11:11Z

src/liquid/app/header.html

I see we removed most of the actions from the nav but i don't see any alternatives introduced for clearing the place list, refreshing the place list, creating places etc

was following this for design thats why options moved away from then header bar

must have missed the clear option though will add that nice catch

Since there's no alternative for the actions in the Users drop down menu i think we should just maintain it for now or provide alternatives

freddieptf · 2024-05-16T18:19:34Z

src/liquid/place/list.html

Can we add an explicit color for the in_progress state? It looks like it defaults to gray which i think is a bleak color for a progress state

freddieptf · 2024-05-16T18:24:12Z

src/liquid/place/directive.html

The places don't get filtered when i click on any of the filters

interesting would like to know how this is tested
what browser are you using?
this works in my chrome and firefox

I am on Firefox, eg if i successfully upload say 10 items from that list and click on the success filter, the list doesn't get updated and i still see all places regardless of state

freddieptf · 2024-05-16T18:47:49Z

src/scss/_tables.scss

+    margin: 0.5em 5px 1em 5px;
+    width: auto;
+    border-radius: $element-border-radius;
+    @include card-shadow;


I don't think we should have a shadow on the table. In this case, the table and the pagination controls should be on the same level yet the table is "above" the pagination controls. I think we should use shadows sparingly

freddieptf · 2024-05-16T19:05:11Z

src/public/scripts/pagination.js

I feel like the UI would be much snappier if we paginated the Place list from the API. I've noticed this triggering the "page is slow or unresponsive" notification on Firefox on large lists. Currently (changes on the main branch) we're lazy loading the place list but i think a solution around pagination would be much better

glad you brought this up
how big is the test dataset?
also curious on how you are testing this be cause it loads very fast even with large datasets on my end since data is loaded once and this pagination is just ui showing and hiding rows

but the reason i used the UI not the api to paginate was the api is slow
you can test this in prod

load a large dataset

filter the dataset, you'll realize the more data you have the slower the filter gets
this is actually what breaks my browser during testing but the pagination works fine

you can feel performance drop with as little as 30-40 rows
Which brings me to this; why are we using session storage to store data? i dont think its the right solution for scaling
we could try browser dbs like pouchDB, indexeddb etc they can offer better performance cc @kennsippell

but also very curious how pagination slow on your end could you share video recording of this?
it not expected to be like this at all

im exploring using the api atm i see your PR introduces table lazy loading it could help

but the reason i used the UI not the api to paginate was the api is slow

Yes, for large lists (1000+) it takes 1-3 seconds to generate the HTML but if we add pagination, we'd only be generating HTML for a smaller dataset making it much faster

filter the dataset, you'll realize the more data you have the slower the filter gets
this is actually what breaks my browser during testing but the pagination works fine

Yes that is what is slowing down my browser as well. If you test with the latest changes It's much better but that doesn't fully fix the problem as we should be trying to keep response times in the 100-200ms range if possible

Which brings me to this; why are we using session storage to store data? i dont think its the right solution for scaling
we could try browser dbs like pouchDB, indexeddb etc they can offer better performance

Right now the server acts as the source of truth since this is a server rendered app, there's no to little state on the client side. It might be the wrong one but not for the reasons you stated, memory will always be faster than anything that does File I/O.

memory is fast for small data but when it comes to large data i think youre better off with a db app becomes memory hog and browser hangs

Memory here is RAM on the server or whatever platform this is deployed on. No data is stored on your browser. For this app, if your browser hangs it's either trying to render a massive amount of HTML or some JS is misbehaving.

we could explore redis as well

I'd prefer if we used an embedded DB (like sqlite3) to reduce deployment complexity but we can discuss this later

until we instruct it to store data locally its ram even in the browser thats why it struggles
anyway ill implement the api pagination and circle back here

@freddieptf I have update pagination to api its works well until pagesize is about 50 rows
but still pretty much usable
this ready for review

its works well until pagesize is about 50 rows

It should be fine for larger lists if you test with NODE_ENV=production. Can we now remove the lazy loading and do pagination? I think the lists loads fast enough now.

if by lazy loading you mean the spinner logic i beg to differ i think its very useful for bad internet/ heavy server loads that might delay response its good practice we should leave it

freddieptf · 2024-06-04T08:11:01Z

src/services/pagination.ts

+  constructor(options: { page: number; pageSize?: number; cookie: {}; requestContactTypeName?: string }) {
+    this.pageSize = options.pageSize ? options.pageSize : 10;
+    this.page = options.page;
+    this.cookiePageSize = this.getPaginationCookie(options.cookie, 'pageSize');


This should belong on the request URL and not in the cookies. It's very transparent what data the app is loading when it's on the page URL. Also, It'd be weird if someone uploaded a list, viewed a random page, cleared the list, uploaded another list then got directed to the previously cached page we find in the cookies.

i want to persist page number in cookie to handle all miscellaneous call to root "/" e.g when you move out to create more users/edit etc you should come back to your "current page"
i can create logic to handle the situation you are talking about

i can create logic to handle the situation you are talking about

Yeah but that's just more code. Personally, I like less code. Having the state in the URL for both the current tab and page is much simpler and easier to understand at a glance both on the UI and in code

i want to persist page number in cookie to handle all miscellaneous call to root "/"

Then maybe we should fix that instead?

whats is much code? :)
you want us to handle all calls to "/"? that sounds like more code than just handling this situation with a variable and an if

cookies are good for storing and persisting user preferences besides there is nothing sensitive we are storing here that the user cant see on the ui

i want to persist page number in cookie to handle all miscellaneous call to root "/"

you want us to handle all calls to "/"?

Find these calls and replace them with appropriate ones i.e redirecting to the correct page. Calling / worked before because there was no pagination

cookies are good for storing and persisting user preferences besides there is nothing sensitive we are storing here that the user cant see on the ui

The current page and active tab are not user preferences, they are part of the navigation state that should be on the URL

cookies have been removed

freddieptf · 2024-06-04T08:14:57Z

src/services/pagination.ts

Not entirely sure if we need this, managing this state would be much easier if all you had to look at was a URL param and slice the list accordingly

i dont understand is this comment intended for the entire pagination file?
you mean slice it in front end? that would be expensive for the PC/browser

This class as used in the endpoint. You can parse the URL param and slice the place list before returning the response to the browser.

not sure i understand this well
what you're explaining i believe is what i'm doing here
is only sliced/paginate and sent back to the user
this has no effect on the original places data in store and also only return sliced data

Not entirely sure if we need this, managing this state would be much easier if all you had to look at was a URL param

This is part of the cookie conversation. We don't need cookies for these

freddieptf · 2024-06-04T08:22:33Z

src/routes/events.ts

@@ -30,6 +30,28 @@ export default async function events(fastify: FastifyInstance) {
      });
    };

+    const updateNavBadge = async () => {


Isn't this already done by the "directive" update?

directive updates the directive, this nav bar is outside the directive

will try move this inside directive and see though it was a bit "shaky" loading the entire nav bar

oh i see so this updates the count on the navbar. You can just remove the count from the navbar then, no need to duplicate what's already on the directive

This might be trickier than i thought. While the filters work, i find them a bit confusing as they represent the state of both lists. This worked before because all the lists were on the same page. In the screenshot below, all the 4 invalid items are on 1 tab so if i apply the invalid filter when on the other tab I get an empty list and have to manually switch tabs to view the invalid items.

I think we should move the filters closer to the table, it would be great if they were directly on the status column. The app only needs to show the user some sort of hint when there are invalid/failed items and these hints can have links that just auto-apply a filter

i see no problem showing 0 rows if the filter is applied as long as we clearly show that there is a filter applied

OK, we can change this behavior later then.

freddieptf · 2024-06-04T08:29:02Z

src/liquid/components/summary.html

What should be on this page? I don't see it used anywhere

this is the summary essentially place states for each contacttype
plan is to handle it later in another pr as this is very big already but shouldnt be a blocker
unless otherwise, i can remove it lmk what you think

If it doesn't have anything on it then there's no need to keep it around

freddieptf · 2024-06-04T08:31:46Z

src/routes/authentication.ts

@@ -23,6 +23,11 @@ export default async function authentication(fastify: FastifyInstance) {

  fastify.get('/logout', unauthenticatedOptions, async (req, resp) => {
    resp.clearCookie(Auth.AUTH_COOKIE_NAME);
+    for (const cookieName in req.cookies) {


Maybe you can remove the call to clearCookie now?

freddieptf · 2024-06-04T08:36:42Z

src/liquid/components/table_header.html

+            <a hx-post="/app/remove-all/{{contactType.name}}" hx-swap="none" 
+            data-toggle="tooltip" data-placement="bottom" title="delete all"
+            hx-confirm="Are you sure you want to DELETE ALL USERS?">
+                <img src="public/icons/delete-all.svg" alt="refresh">


Can we use the same icon used on the list items?

freddieptf · 2024-06-04T08:37:36Z

src/liquid/place/directive.html

Fix indent please

freddieptf · 2024-06-04T08:43:42Z

src/liquid/place/place_navbar.html

+</div>
+
+<script>
+    function highlight(element) {


Can we find a way to update this state from the server? Let's try and avoid JS like this as much as possible

Anything ui related that can be done frontend shouldnt do around trip to the server, find it unnecessary

While i agree with you, you still make the trip to the server to fetch the page after clicking the tab. Being a server rendered app, the server manages most of the state in the UI. In this case, you can return both the page and the updated navbar in the response from the server and achieve the same effect without JS

but htmx, sse etc all run in the frontend and do manipulate ui before even hitting the server!
this is equivalent to asking the server to register/listen for button clicks
i dont think thats what SSR is
otherwise you end up with things ive seen here where server is storing and assigning classes

but htmx, sse etc all run in the frontend and do manipulate ui before even hitting the server!
this is equivalent to asking the server to register/listen for button clicks

There's nothing fancy going on. We're just issuing AJAX calls to the server on certain events. htmx helps with this and content loading/swapping. We can ditch htmx and just use full page reloads for any state change and the app would work the same as the state is all on the server.

ditching it would be pointless. you can never get rid of js completely in web development even if your back end is python
JS was built for web development
The point i was making wasnt about htmx only but all the js served in the public folder all of it runs in the browser
all the calendar js tooltip js etc

Yes i understand some UI elements are complex and need JS to manage their state. To be clear, I'm not saying we should get rid of JS, I'm saying that the application state should be driven by the server as much as possible. Application state in this app should be the same with or without JS

UI state and server state should be managed seperately, with only communication between the 2 on the state of one another

All I'm saying is update the tab's state by the same way the contents of the page are updated. We don't need a script to do that.

this has been changed

freddieptf · 2024-06-04T08:52:57Z

src/css/custom-bootstrap.css

Interesting that you still rely on bootstrap. Did bulma not provide this?

i can customized bootstrap to get only the elements i want for this its tooltips, badges and breadcrumbs keeping the file very small
not sure you can do that with bulma

src/liquid/components/place_item.html

src/liquid/app/view.html

jonathanbataire · 2024-06-10T07:23:12Z

@freddieptf lmk when you have time we do a peer review online for this PR so i can better understand somethings
I can be available thursday/friday

jonathanbataire · 2024-07-04T09:06:12Z

@freddieptf requesting review here this is ready

freddieptf · 2024-07-04T16:23:55Z

src/liquid/app/header.html

+</nav>
+
+<script>
+    document.addEventListener('click', function () {


Not sure why this listener is added to the document and not the element being targeted

aah i see, to hide the drop down menu when we click anywhere on the page, correct?

Now I'm confused, since the drop down disappears when it's not hovered on, why is this needed?

freddieptf · 2024-07-04T16:25:53Z

src/liquid/app/header.html

+    </ul>
+</nav>
+
+<script>


It'd be great to keep scripts like this in an app.js or something

freddieptf · 2024-07-12T16:14:59Z

Something i noticed when the list had one item. The tool tips get cut off and there's a scrollbar

Also, the Move button should be an item under the drop down in the first button

jonathanbataire marked this pull request as draft May 11, 2024 17:17

jonathanbataire commented May 11, 2024

View reviewed changes

jonathanbataire marked this pull request as ready for review May 14, 2024 09:30

jonathanbataire requested review from kennsippell, freddieptf and paulpascal May 14, 2024 09:30

This was referenced May 14, 2024

Improve the UI for login #157

Open

UI improvements - forms #172

Open

freddieptf requested changes May 16, 2024

View reviewed changes

freddieptf reviewed May 16, 2024

View reviewed changes

v1

c36c9c9

jonathanbataire force-pushed the 131-main-ui branch from c67e457 to c36c9c9 Compare May 30, 2024 18:15

jonathanbataire added 3 commits May 31, 2024 18:50

v2

91eb75f

v3

b989819

working update

5e5759d

jonathanbataire changed the base branch from 117-login-ui to main June 1, 2024 15:28

jonathanbataire requested a review from freddieptf June 1, 2024 15:29

jonathanbataire added 9 commits June 1, 2024 18:32

remove header title

029b853

clean up

42e64da

clean up loading table

defce26

update nav counter on placechange

aa9f60a

🎉

6a156fa

clear cookies on logout

f5d0624

💥 🔨

de6905c

clean up

0e9bdd8

💥

fcb1d52

freddieptf requested changes Jun 4, 2024

View reviewed changes

jonathanbataire commented Jun 4, 2024

View reviewed changes

src/liquid/app/view.html Outdated Show resolved Hide resolved

jonathanbataire commented Jun 4, 2024

View reviewed changes

src/liquid/app/view.html Show resolved Hide resolved

address feedback

91f774f

jonathanbataire requested a review from freddieptf June 7, 2024 07:43

jonathanbataire added 2 commits June 7, 2024 11:53

🔨

9dea8f8

css clean up

deca6c0

jonathanbataire added 4 commits June 11, 2024 11:59

remove cookies

6ba45f0

clean up

5601185

clea

b00cd6c

clean up

3e74c80

freddieptf reviewed Jul 4, 2024

View reviewed changes

main UI - directives and table views #168

Are you sure you want to change the base?

main UI - directives and table views #168

Conversation

jonathanbataire commented May 11, 2024 • edited Loading

Description

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

jonathanbataire May 17, 2024 • edited Loading

Choose a reason for hiding this comment

freddieptf May 17, 2024 • edited Loading

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

freddieptf May 16, 2024 • edited Loading

Choose a reason for hiding this comment

jonathanbataire May 17, 2024 • edited Loading

Choose a reason for hiding this comment

jonathanbataire May 17, 2024 • edited Loading

Choose a reason for hiding this comment

jonathanbataire May 17, 2024 • edited Loading

Choose a reason for hiding this comment

freddieptf May 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanbataire May 17, 2024 • edited Loading

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

jonathanbataire Jun 6, 2024 • edited Loading

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

jonathanbataire Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanbataire Jun 7, 2024 • edited Loading

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

jonathanbataire Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freddieptf Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

jonathanbataire Jun 6, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanbataire Jun 6, 2024 • edited Loading

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

jonathanbataire Jun 6, 2024 • edited Loading

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

jonathanbataire commented May 11, 2024 •

edited

Loading

jonathanbataire May 17, 2024 •

edited

Loading

freddieptf May 17, 2024 •

edited

Loading

freddieptf May 16, 2024 •

edited

Loading

jonathanbataire May 17, 2024 •

edited

Loading

jonathanbataire May 17, 2024 •

edited

Loading

jonathanbataire May 17, 2024 •

edited

Loading

freddieptf May 17, 2024 •

edited

Loading

jonathanbataire May 17, 2024 •

edited

Loading

jonathanbataire Jun 6, 2024 •

edited

Loading

jonathanbataire Jun 4, 2024 •

edited

Loading

jonathanbataire Jun 7, 2024 •

edited

Loading

jonathanbataire Jun 4, 2024 •

edited

Loading

freddieptf Jun 5, 2024 •

edited

Loading

jonathanbataire Jun 6, 2024 •

edited

Loading

jonathanbataire Jun 6, 2024 •

edited

Loading

jonathanbataire Jun 6, 2024 •

edited

Loading

jonathanbataire commented Jul 4, 2024 •

edited

Loading

freddieptf commented Jul 12, 2024 •

edited

Loading