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

PLEASE REVIEW - 0.4 Release Candidate #61

Closed
2 tasks done
seanhess opened this issue Jan 3, 2025 · 11 comments
Closed
2 tasks done

PLEASE REVIEW - 0.4 Release Candidate #61

seanhess opened this issue Jan 3, 2025 · 11 comments

Comments

@seanhess
Copy link
Owner

seanhess commented Jan 3, 2025

Hey all, @sfwc @kfigiela @benjaminweb @Skyfold @cgeorgii @tusharad

0.4 is ready, with all our latest changes and lots of new documentation.

https://hackage.haskell.org/package/hyperbole-0.4.2/candidate/docs/Web-Hyperbole.html

Make sure to check out the new examples site if you haven't seen it yet

https://docs.hyperbole.live

Please review if you have time, and let me know if you notice any issues, big or small.

Thanks for all your help and contributions!

TODO

  • Update inline documentation examples to 0.4
  • Update CHANGELOG
@Skyfold
Copy link
Contributor

Skyfold commented Jan 3, 2025

Would you be interested in using doctest-parallel for the non-embeded examples? I would be happy to set it up.

@benjaminweb
Copy link
Contributor

benjaminweb commented Jan 3, 2025

First, thanks for your awesome work and craft you did with hyperbole.
I appreciate your responsiveness in realising requirements as they pop up.

Not necessarily for 0.4.0 release, but here’s my take on the current state:

  • Documentation: Let me know if you want me to incorporate my rewrite of the tutorial into a pull request.
  • Documentation, visual: Does a graphical representation of the architecture make sense for newcomers? If, what would be a first draft? Let me know if you want me to make amendments to my draft.
  • Documentation: I’ve found some of your answers to my questions in the todo mvc pretty clarifying for me; you might double check if you’d like to include some as docstrings for the functions of the example / for the definitions of the general entities of Hyperbole (I haven’t checked whether you already might have done this).
  • Web.View: table cell, breaking it further down by using row and columns didn’t work for me; how do I do that? don’t have a code example handy right now.
  • Tables, requirement: Align numbers per (custom) decimal separator.
  • UI Interaction Requirement: Put cursor on load / on specific action into a specific input field.

To which specifics do you want feedback?

@seanhess
Copy link
Owner Author

seanhess commented Jan 3, 2025

Would you be interested in using doctest-parallel for the non-embeded examples? I would be happy to set it up.

Argh, I totally forgot to update all the embedded examples for 0.4. The point of bin/docgen and the embedded examples was to make it so out-of-date documentation doesn't compile. Is that redundant with doctest-parallel? If so, should we just port all the inline examples to embedded ones, or is there a reason to use doctest-parallel instead?

@seanhess
Copy link
Owner Author

seanhess commented Jan 3, 2025

  • Documentation: Let me know if you want me to incorporate my rewrite of the tutorial into a pull request.

I've been incorporating your feedback into the docs. Note that I stopped calling anything "components" in the latest version. Please feel free to create a PR with anything you think would help. I'd prefer to keep PRs small unless we need to discuss a major effort. What do you mean by incorporate the tutorial into a PR? I'd be happy to feature a link to a tutorial you author.

  • Documentation, visual: Does a graphical representation of the architecture make sense for newcomers? If, what would be a first draft? Let me know if you want me to make amendments to my draft.

I think 0.4 has been delayed enough. Let's get it out without a visual aid. I'm open to creating one, but let's discuss that elsewhere.

  • Documentation: I’ve found some of your answers to my questions in the todo mvc pretty clarifying for me; you might double check if you’d like to include some as docstrings for the functions of the example / for the definitions of the general entities of Hyperbole (I haven’t checked whether you already might have done this).

Can you make a list of which ones you thought were most helpful? I've tried to incorporate some of your questions into the introduction on hackage.

  • Web.View: table cell, breaking it further down by using row and columns didn’t work for me; how do I do that? don’t have a code example handy right now.

Did you see the Data Table Example I added this morning? Does that help?

  • Tables, requirement: Align numbers per (custom) decimal separator.

Is this as simple as a monospaced font and Numeric.showFFloat? What am I missing here?

  • UI Interaction Requirement: Put cursor on load / on specific action into a specific input field.

Open an issue for this? Might be a use case for #25

@Skyfold
Copy link
Contributor

Skyfold commented Jan 4, 2025

Would you be interested in using doctest-parallel for the non-embeded examples? I would be happy to set it up.

Argh, I totally forgot to update all the embedded examples for 0.4. The point of bin/docgen and the embedded examples was to make it so out-of-date documentation doesn't compile. Is that redundant with doctest-parallel? If so, should we just port all the inline examples to embedded ones, or is there a reason to use doctest-parallel instead?

I recommend we use a combination of both, since at their core they are designed for different purposes. doctest-parallel is made for code you can paste in ghci, while your embed method is designed for whole modules/functions you paste in your editor. It is ugly to make doctest-parallel work well for whole module examples since that entire code block must be pasted in a ghci session. I also like that the documentation and examples web server share the same code, making the link between what you see and how to implement it clear.

My only suggestion would be to add a module embed, that way the whole example is type checked. For example:

{\-# LANGUAGE DeriveAnyClass #-\}
{\-# LANGUAGE OverloadedStrings #-\}
{\-# LANGUAGE TypeFamilies #-\}
{\-# LANGUAGE DataKinds #-\}

module Main where

import Web.Hyperbole

#EMBED Example/Intro/BasicPage.hs main

#EMBED Example/Intro/BasicPage.hs messagePage

Each #EMBED will be tested, but the whole example might not work (incorrect imports, missing LANGUAGE pragma).

We can use doctest-parallel for inline examples like:

>>> :{
myDocument :: ByteString -> ByteString
myDocument content =
  [i|<html>
    <head>
      <title>#{title}</title>
      <script type="text/javascript">#{scriptEmbed}</script>
      <style type="text/css">#{cssResetEmbed}</style>
    </head>
    <body>#{content}</body>
  </html>|]
:}

Or for short functions:

>>> :{
app :: Application
app = do
  liveApp
    toDocument
    (runApp . routeRequest $ router)
 where
  runApp :: (Hyperbole :> es, IOE :> es) => Eff (Concurrent : Todos : es) a -> Eff es a
  runApp = runTodosSession . runConcurrent
:}

The 0.4 release does not have to wait until we add doctest-parallel for testing. I'll work on modifying the inline examples to fit the doctest-parallel syntax and we can see how it will work in practice.

@cgeorgii
Copy link
Contributor

cgeorgii commented Jan 4, 2025

Nice! I've opened a few very minor PRs and will keep checking it out for a bit longer but it's looking good in my opinion! :)

@benjaminweb
Copy link
Contributor

benjaminweb commented Jan 4, 2025

@seanhess I am trying to just factor out a static view. What's needed to make it result in a Page?
Context: I am rewriting the docs for a PR to introduce View functions before HyperViews. Reason: The documentation should introduce one concept after another, currently it does two: Factoring out Views and upgrading them as a HyperView.

{-# OPTIONS_GHC -Wno-missing-signatures #-}

module Example.Intro.BasicPage where

import Web.Hyperbole
import Web.Hyperbole.HyperView
import Web.Hyperbole.Effect.Handler

main = do
  run 3000 $ do
    liveApp (basicDocument "Example") (loadToResponse messagePage')

{-
messagePage :: Eff es (Page '[])
messagePage = do
  pure $ do
    col (pad 10) $ do
      el bold "Hello World"
-}

staticView :: (ViewId id) => id -> View id () -> View ctx ()
staticView vid = el (att "id" (toViewId vid) . flexCol) . addContext vid

messagePage' :: Eff es (Page '[Message])
messagePage' = pure $ staticView Message messageView

data Message = Message
  deriving (Show, Read, ViewId)

messageView :: View Message ()
messageView =
  col (pad 10) $ do
    el bold "Hello World"

So, either I am not aware of what builtin functionality should do what I need (then how should it be documented?) OR the functionality for static views does not exist.

If the functionality (for pages with only using static views) does not exist:

  • Isn't it possible to get rid of staticView at all (by making the translation of View id () to View ctx () automatic?
  • Ideally, how could loadToResponse be unified with runPage? runPage seems not to compile for me when there is no HyperView at all.
  • If the assumption is that there always is a HyperView on any page, this should be documented!

In any way, this was the reason why I felt forced to use HyperViews where I didn't need independent updatable views!

@seanhess
Copy link
Owner Author

seanhess commented Jan 4, 2025

The 0.4 release does not have to wait until we add doctest-parallel for testing. I'll work on modifying the inline examples to fit the doctest-parallel syntax and we can see how it will work in practice.

Sounds great. We DO need to get the inline examples updated for 0.4 before release. Why don't we do that first, then create a PR after with the doctest-parallel to see what it adds.

@seanhess
Copy link
Owner Author

seanhess commented Jan 4, 2025

@benjaminweb It sounds like you're running in to out-of-date inline documentation. loadToResponse doesn't exist anymore. I'll get that updated pronto. Here's the basic example from Example.Intro.BasicPage:

main :: IO ()
main = do
  run 3000 $ do
    liveApp (basicDocument "Example") (runPage messagePage)

messagePage :: Eff es (Page '[])
messagePage = do
  pure $ do
    col (pad 10) $ do
      el bold "Hello World"

Page is just a type alias for a "root view".

type Page views = View (Root views) ()

So the messagePage function is expected to return a view that matches it's context: Root '[]. You can fix your example by making a view with a generic context:

staticView :: View anything ()
staticView = do
  col (pad 10) $ do
    el_ bold "Hello World"
    
messagePage :: Eff es (Page '[])
messagePage = do
  pure staticView

@seanhess
Copy link
Owner Author

seanhess commented Jan 4, 2025

@benjaminweb would it be easier to understand if we changed the name of Root to page and got rid of the type alias?

It would make the type signature of pages more complicated, but more explicit and clear.

main :: IO ()
main = do
  run 3000 $ do
    liveApp (basicDocument "Example") (runPage messagePage)

messagePage :: Eff es (View (Page '[]) ())
messagePage = do
  pure $ do
    col (pad 10) $ do
      el bold "Hello World"

Then it might be more obvious how to make a static view:

messagePage :: Eff es (View (Page '[]) ())
messagePage = do
  pure staticView
  
-- could also be typed `View a ()`, because pageView doesn't depend on the context or use any actions
pageView :: View (Page '[]) ()
pageView = do
    col (pad 10) $ do
      el bold "Hello World"

@benjaminweb
Copy link
Contributor

benjaminweb commented Jan 4, 2025

  • Documentation: Let me know if you want me to incorporate my rewrite of the tutorial into a pull request.

I've been incorporating your feedback into the docs. Note that I stopped calling anything "components" in the latest version. Please feel free to create a PR with anything you think would help. I'd prefer to keep PRs small unless we need to discuss a major effort. What do you mean by incorporate the tutorial into a PR? I'd be happy to feature a link to a tutorial you author.

  • see overhaul documentation #64, it might benefit from
    • a 'common pitfalls' / FAQ section
    • further polishing of headlines, ordering of docstrings section
  • Documentation, visual: Does a graphical representation of the architecture make sense for newcomers? If, what would be a first draft? Let me know if you want me to make amendments to my draft.

I think 0.4 has been delayed enough. Let's get it out without a visual aid. I'm open to creating one, but let's discuss that elsewhere.

Created #67.

  • Documentation: I’ve found some of your answers to my questions in the todo mvc pretty clarifying for me; you might double check if you’d like to include some as docstrings for the functions of the example / for the definitions of the general entities of Hyperbole (I haven’t checked whether you already might have done this).

Can you make a list of which ones you thought were most helpful? I've tried to incorporate some of your questions into the introduction on hackage.

Incorporated into the paragraphs with #64, yet not included in the docstrings themselves. Also see the definition list under Entities (in the generated docs).

  • Web.View: table cell, breaking it further down by using row and columns didn’t work for me; how do I do that? don’t have a code example handy right now.

Did you see the Data Table Example I added this morning? Does that help?

  • Tables, requirement: Align numbers per (custom) decimal separator.

Is this as simple as a monospaced font and Numeric.showFFloat? What am I missing here?
I've looked into Numeric.showFFloat, but don't see how it realises this with (or even better without zeros (if we let Hyperbole replace it with spaces).

0003.333
3333.33
  • UI Interaction Requirement: Put cursor on load / on specific action into a specific input field.

Open an issue for this? Might be a use case for #25

Created #66.

@benjaminweb would it be easier to understand if we changed the name of Root to page and got rid of the type alias?
That wouldn't clarify it to the extent making the new complication worth it, IMHO. How about creating a type synonym StaticView that is just View a ()? I mean it distinguishes it clearly from View MyNiceView ()?


Additionally:

TODO:

  • check QueryParams / add QueryParams example

@seanhess seanhess closed this as completed Jan 9, 2025
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

No branches or pull requests

4 participants