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

Statistics display #91

Merged
merged 2 commits into from
Jan 25, 2024
Merged

Statistics display #91

merged 2 commits into from
Jan 25, 2024

Conversation

jpodivin
Copy link
Collaborator

We need to decide on our report collection target before merging this. Something that is ambitious, yet reachable.
Until we know for sure we shouldn't merge it.

@jpodivin jpodivin force-pushed the stats branch 3 times, most recently from c444326 to 897b3f4 Compare January 22, 2024 09:14
Copy link
Collaborator

@TomasTomecek TomasTomecek left a comment

Choose a reason for hiding this comment

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

Very nice!

I advocate for 1000 by Mar 31st

@jpodivin jpodivin changed the title WIP - Statistics display Statistics display Jan 22, 2024
backend/store.py Outdated Show resolved Hide resolved
@praiskup
Copy link
Member

I'd propose 500 to make it realistic (100 is going to exist by the end of January).

Copy link
Member

@FrostyX FrostyX left a comment

Choose a reason for hiding this comment

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

I added some suggestions for improving the design. It would look like this:

Screenshot_2024-01-23_11-33-11

backend/store.py Outdated Show resolved Hide resolved
frontend/public/css/style.css Outdated Show resolved Hide resolved
frontend/src/app/homepage.cljs Outdated Show resolved Hide resolved
frontend/src/app/homepage.cljs Outdated Show resolved Hide resolved
@jpodivin jpodivin force-pushed the stats branch 2 times, most recently from e494054 to a4bf8f8 Compare January 23, 2024 14:05
@jpodivin jpodivin requested review from nikromen and FrostyX January 23, 2024 14:07
Copy link
Member

@FrostyX FrostyX left a comment

Choose a reason for hiding this comment

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

Thank you very much for the update. A couple of minor things and then we are ready to go.

backend/api.py Outdated Show resolved Hide resolved
(float
(/
(:total_reports @backend-stats)
@report-target)) 100) 100))
Copy link
Member

Choose a reason for hiding this comment

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

We will probably need to round the number. In my case backend returns a reasonable number:

{"total_reports":7}

but frontend prints

Are we there yet?
0.7000000000000001%

frontend/src/app/homepage.cljs Outdated Show resolved Hide resolved
[:div {
:id "progress"
:style {:width (str (progress-width) "%")}
}]])
Copy link
Member

Choose a reason for hiding this comment

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

I'll need to set up some linter for the formatting. In comparison to e.g. python, AFAIK all lisps do:

  • Ending brackets are not on their own lines. All of them are at the end of the last command
  • We don't write opening bracket and then newline. e.g. :div {\n. The { goes to the same line as whatever comes after it

So for example:

(defn render-stats []
  [:div {:id "progressbar"}
   [:h5 "Are we there yet?"]
   [:div {:id "progressbar-number"}
    [:p (progress-width) "%"]]
   [:div
    {:id "progress"
     :style {:width (str (progress-width) "%")}}]])

Especially the ending brackets are incorrect on multiple places

@jpodivin jpodivin force-pushed the stats branch 2 times, most recently from 7a5423b to 0c17e0c Compare January 23, 2024 15:57
Signed-off-by: Jiri Podivin <[email protected]>
@FrostyX FrostyX merged commit 96b51a3 into fedora-copr:main Jan 25, 2024
2 checks passed
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.

5 participants