Refactor the loading and request management #230

turboMaCk · 2020-11-19T17:41:20Z

resolves #224

TODO:

Unify the source of truth between loading and request sending
Update pagination for compatibility with new approach
Update sorting for compatibility with new approach

This changes the way we handle Loading:

result : RemoteData is now source of truth for if we should or should not trigger the search
resut = Loading is set only when right conditions are met (eg term is not empty)
Main.elm queries data only when result state is Loading
Unlike before, we now need to set Loading also for pagination. Because of this I made a small layout changes so that pagination is visible even in loading state for better UI.

This PR could be improved further - for instance we can avoid having prev button enabled during loading etc. However I didin't want to introduce way too many changes in single PR.

github-actions · 2020-11-19T17:43:20Z

🚀 Deployed on https://5fbe88465744a5110f1ccc43--nixos-search.netlify.app

turboMaCk · 2020-11-21T20:59:18Z

src/Main.elm

+            if Search.shouldLoad searchModel then
+                submitQuery PackagesMsg Page.Packages.makeRequest searchModel


single source of truth for when to perform loading.

turboMaCk · 2020-11-21T20:59:56Z

src/Main.elm

+                    , case href of
+                        -- ignore links with no `href` attribute
+                        "" ->
+                            Cmd.none
+
+                        _ ->
+                            Browser.Navigation.load href


this fixes issues with links lacking Html.Attributes.href reloading app.

turboMaCk · 2020-11-21T21:00:31Z

src/Search.elm

+ensureLoading : Model a -> Model a
+ensureLoading model =
+    if model.query /= Nothing && model.query /= Just "" && List.member model.channel channels then
+        { model | result = RemoteData.Loading }
+
+    else
+        model


this is the only place where we explicitely define in which case we are able to load the data.

I think some condition is missing since we shouldn't load when we open details of an search result item.

in that case we simply don't call this function.

turboMaCk · 2020-11-21T21:01:35Z

src/Search.elm

+                            in
+                            ChangePage <| ((total // model.size) - remainder) * model.size
+                    ]
+                    [ text "Last" ]


most of the diff here is due to changes in indentation due to Html.map.

turboMaCk · 2020-11-21T21:03:11Z

src/index.less

@@ -1,6 +1,7 @@
 body {
  position: relative;
  min-height: 100vh;
+  overflow-y: scroll;


This is to prevent page jumping few pixels in width. Having results might force scrollbars while loading state does not (list is very short). Setting vertical scrollbar to be always visible means no changes in document width as these two state change.

turboMaCk · 2020-11-21T21:03:55Z

src/index.less

+.loader-wrapper {
+  height: 200px;
+  overflow: hidden;
+}


this is to prevent jumping due to changes in height as animation is running

turboMaCk · 2020-11-21T21:06:30Z

src/index.less

+.sort-form,
+.sort-form > .sort-group {
+  margin-bottom: 0;
+}
+.pager {
+  & > li > a {
+    cursor: pointer;
+    margin: 0 2px;
+  }
+}


Sorry I could not resist my OCD alarm. This is just to clean up the layout just a tiny bit.

before

now:

garbas

Just did a quick test and I noticed that when you click on a result item a new request is sent to the backend. I think we messed up this once and I'm not sure I'm going to remember to check this always. We should make this somehow more reliable, but I have zero idea how.

garbas · 2020-11-23T23:17:41Z

src/Main.elm

-    -> ( Model, Cmd Msg )
-submitQuery old ( new, cmd ) =
+attemptQuery : ( Model, Cmd Msg ) -> ( Model, Cmd Msg )
+attemptQuery (( model, _ ) as pair) =


Didn't know you can use as in this way. Neat!

garbas · 2020-11-23T23:19:58Z

src/Main.elm

-                    , Browser.Navigation.load href
+                    , case href of
+                        -- ignore links with no `href` attribute
+                        "" ->


Will this work properly also for links with "#someid"?

garbas · 2020-11-23T23:28:32Z

There is also another thing that it would be nice to do, while we are at it. When a result item is selected, we should scroll to it. I think this was working at some point or maybe I was just dreaming that I implemented it :) This is in a reference to this bug report #231

resolves #224

turboMaCk · 2020-11-25T16:22:57Z

good catch.

The problem is:

open detail triggers urlChange effect
effects is handled in Main
in main init is called which clears the results

These sum type page models are quite tricky to work with 😰 . We need to prevent init from being called. in situations like this.

turboMaCk self-assigned this Nov 19, 2020

turboMaCk marked this pull request as draft November 19, 2020 17:41

turboMaCk commented Nov 21, 2020

View reviewed changes

turboMaCk marked this pull request as ready for review November 21, 2020 21:06

turboMaCk requested a review from garbas November 21, 2020 21:08

garbas requested changes Nov 23, 2020

View reviewed changes

garbas reviewed Nov 23, 2020

View reviewed changes

turboMaCk added 5 commits November 25, 2020 17:14

Refactor the loading and request management

20b14b0

resolves #224

fix pagination

6edf846

simpler pagination

1425098

improve loading behavior

636a5de

remove uncessary change

327a8aa

turboMaCk force-pushed the turboMaCk/fix-loading branch from 8e45cb3 to 327a8aa Compare November 25, 2020 16:14

turboMaCk marked this pull request as draft November 25, 2020 16:33

throw away init if page doesn't change during url transition

afc36e2

turboMaCk marked this pull request as ready for review November 25, 2020 16:35

turboMaCk requested a review from garbas November 25, 2020 16:35

garbas approved these changes Nov 28, 2020

View reviewed changes

garbas merged commit 060daed into master Nov 28, 2020

garbas deleted the turboMaCk/fix-loading branch November 28, 2020 23:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the loading and request management #230

Refactor the loading and request management #230

turboMaCk commented Nov 19, 2020 •

edited

github-actions bot commented Nov 19, 2020 •

edited

turboMaCk Nov 21, 2020

turboMaCk Nov 21, 2020 •

edited

turboMaCk Nov 21, 2020

garbas Nov 23, 2020

turboMaCk Nov 25, 2020

turboMaCk Nov 21, 2020

turboMaCk Nov 21, 2020

turboMaCk Nov 21, 2020

turboMaCk Nov 21, 2020

garbas left a comment

garbas Nov 23, 2020

garbas Nov 23, 2020

garbas commented Nov 23, 2020

turboMaCk commented Nov 25, 2020

		if Search.shouldLoad searchModel then
		submitQuery PackagesMsg Page.Packages.makeRequest searchModel

Refactor the loading and request management #230

Refactor the loading and request management #230

Conversation

turboMaCk commented Nov 19, 2020 • edited

github-actions bot commented Nov 19, 2020 • edited

Choose a reason for hiding this comment

turboMaCk Nov 21, 2020 • edited

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

garbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garbas commented Nov 23, 2020

turboMaCk commented Nov 25, 2020

turboMaCk commented Nov 19, 2020 •

edited

github-actions bot commented Nov 19, 2020 •

edited

turboMaCk Nov 21, 2020 •

edited