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

Ospedale/hello world #1431

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Ospedale/hello world #1431

wants to merge 6 commits into from

Conversation

kaol
Copy link
Member

@kaol kaol commented Jun 15, 2023

No description provided.

@kaol kaol marked this pull request as draft June 15, 2023 12:23
{loading: Just _} -> Spinner.loadingSpinner
{session: Nothing} -> loginComponent {setSession: setSession <<< Just}
{session: Just s} -> renderSite (Just s.session)
]
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is almost 100 lines. Would it make sense to factor it into smaller pieces?

]

jsApp :: {} -> JSX
jsApp = unsafePerformEffect app
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know about PureScript, but I think that unsafePerformIO is generally regarded as an antipattern in Haskell.

let publishingTime :: Maybe LocalDateTime
publishingTime =
if not publish then Nothing
else (_.publishingTime =<< inputArticle) <|> nowLocal
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use guards instead of if here.

where
-- Assume only HTML elements
buildBody (Html html) = html
buildBody _ = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, we have quite a lot of code. Personally, I'd prefer smaller pieces.

Copy link
Contributor

@jllang jllang left a comment

Choose a reason for hiding this comment

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

This is a rather large PR, but it might not make sense to try chopping it into smaller pieces. I find this code generally OK, though I didn't focus on details. Does Ospedale run with the usual yarn install, yarn build and yarn start mantra? Maybe a README.md containing installation instructions would be nice.

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.

2 participants