-
Notifications
You must be signed in to change notification settings - Fork 12
WIP: Add external css example #102
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
base: main
Are you sure you want to change the base?
WIP: Add external css example #102
Conversation
examples/Example/App.hs
Outdated
@@ -75,19 +80,57 @@ import Web.Hyperbole.Effect.Server (Request (..)) | |||
run :: IO () | |||
run = do | |||
hSetBuffering stdout LineBuffering | |||
putStrLn "Starting Examples on http://localhost:3000" | |||
|
|||
port <- do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put any unreleated improvements / refactors into a separate PR
examples/Example/App.hs
Outdated
|
||
-- Use the embedded version for real applications (see basicDocument). | ||
-- The link to /hyperbole.js here is just to make local development easier | ||
toDocument :: BL.ByteString -> BL.ByteString |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to branch here. You could either load your custom css on every page by modifying toDocument
, or, even better, do everything in TodoCSS.page:
page :: (Todos :> es) => Eff es (Page '[TodosView, TodoView])
page = do
todos <- Todos.loadAll
pure $ tag "div" id $ do
stylesheet "https://cdn.jsdelivr.net/npm/[email protected]/index.min.css"
-- the rest of your page
...
That way, your stylesheet is loaded on your page, and nothing else. All without modifying App.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.... The issue is that Example.App.hs is serving double-duty as an example of how to bootstrap an application. I'd really like to avoid anything as complex as a branch like this if possible.
This CSS is just a reset. Are you sure it would conflict? If it will definitely mess it up, maybe let's put this in its own app. I'm open to better ideas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with that assumption but I didn't actually try it. I'll keep you posted.
examples/Example/Page/Todos/Todo.hs
Outdated
|
||
data AllTodos = AllTodos | ||
data TodosView = MkTodosView |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Careful not to rename / refactor things to your preference in this PR. It'll make it hard to separate merging the example from a discussion on preferred naming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I said in the description
Currently, I've renamed some types, e.g. AllTodos => MkTodosView to facilitate my own understanding, but I'll revert this at the end.
It looks like you didn't see it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it thanks!
page :: (Todos :> es) => Eff es (Page '[TodosView, TodoView]) | ||
page = do | ||
todos <- Todos.loadAll | ||
pure $ tag "div" id $ do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment in App.hs, but you can load your stylesheet here:
pure $ tag "div" id $ do
stylesheet "https://cdn.jsdelivr.net/npm/[email protected]/index.min.css"
tag "h1" id $ text "Todos CSS"
...
todos <- Todos.loadAll | ||
pure $ tag "div" id $ do | ||
tag "h1" id $ text "Todos CSS" | ||
tag "p" id $ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use tag
repeatedly? Define aliases for any tags you are missing:
p :: Mod id -> View id () -> View id ()
p = tag "p"
todoForm filt = do | ||
let f :: TodoForm FieldName = fieldNames | ||
tag "div" id $ do | ||
tag "span" id $ do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "span" necessary here? (Does the CSS specifically target span?) Why not just use el
?
Perhaps you are also expressing a preference to control the exact html tags used / semantic DOM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I will definitely create nicely named functions.
By opening the PR in draft mode, my intention was to get early feedback regarding the way I managed to inject the CSS rules for this specific page, and also maybe to get advice and sharing hyperview code wich looks tricky (but you already answered that, so I'll just duplicate code for now)
So the UI is not done currently, I've only implemented the bare essentials to move forward.
great start! I added some comments and questions. I don't see any use of the external stylesheet yet? I'm also not sure the best way to include both examples without code duplication. I'd really prefer not to duplicate any non-external-stylesheet code if possible, so the examples don't get out of sync. I'd also prefer to avoid making the examples too complicated or generalized. Since our TodoMVC with external CSS example is an extension of the normal one, maybe it will help it have it import the original example? Then at least you don't need the .Shared module, you can keep them in the original example and import from there. |
- need to fix CSS on edit input - need to add a link to go back to the previous page
ce2c298
to
545106c
Compare
I'm happy with the result currently @seanhess Maybe you can take a look to see if there are any snags left? And if not maybe as a last step I can try to fuse both examples into the same module and remove the shared module as discussed? Or maybe it's good enough with that bit of duplication? I suspect the module itself may be a bit messy after that, so I reckon it's a good point to take a look now. Then I can try that last change or revert if it turns out not to be better. |
hyper AllTodos $ todosView FilterAll todos | ||
|
||
--- AllTodos ---------------------------------------------------------------------------- | ||
--- TodosView ---------------------------------------------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function seemed old/unused so I deleted it.
FIXME: but since this example is meant to match as close as possible to the original CSS version | ||
FIXME: and not diverge too much from the other todo example, I'm leaving as-is. | ||
-} | ||
. att "autocomplete" "off" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should go fix the source for input
, right? Probably set to "off" most of the time. (Right now input TextInput
, which is in the main example, sets it to "text-input" which is wrong).
I wanted to make it easy to set "type" and "autocomplete" with one type, but maybe it wasn't a great choice?
where | ||
filterLi f str = | ||
li' (extClass "filter" . selectedFilter f) $ do | ||
a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you avoid using Web.View.link
deliberately? Or just not know about it? Web.View.Types.Url
has an IsString
instance:
link "" (onClick (Filter f)) (text str)
Cool, thanks for doing this! I'll can play with it from here (I need to think about how to present this in a clear way that shows the minimum necessary changes between the two examples). The Big Question: how did it go? Were you fighting the framework too much? Which helpers or tools would make your experience smoother? |
Great.
It went great! I definitely want to play with the library some more. As a learner of the language still, I've been on the lookout for a good Haskell web library for a while, and Hyperbole feels nice because it brings something to the table that's quite different from more traditional solutions. I've played a fair bit with Elixir/Phoenix prior to learning Haskell, but didn't feel compelled to invest into that technology because I was more interested in having a solid language rather than a cool tech. But I'm quite keen to explore a server-first approach to reactivity, it feels quite effective. The only thing were I felt friction was the global import, that kinda made my understanding a bit muddy at the start. What I'd like to understand next is:
|
Hey @benjamin-thomas, sorry to pull the rug out from under you, but I tagged you in a big refactor PR for web-view (now atomic-css). It'll directly affect your example. seanhess/atomic-css#21 We're getting rid of the global import, but I'm not 100% sure what to replace it with. The "default" utility-based approach will be to import both hyperbole and atomic-css:
If you import only Hyperbole, you'll have access to the LMK if you have any questions, but after reviewing the PR can you update this? |
Hi @seanhess, Yeah no problem! I intend to look at the refactor PR this week-end. Then I'll bring this one up to date once it's ready. I'll keep you posted |
Attempt to demonstrate how one could use Hyperbole, with just CSS, see #100
The idea is to demonstrate side-by-side the same implementation of the todomvc example.
For the moment, I've managed to inject a custom head via path detection before the library code starts.
I've then extracted common logic to a "shared" module, but I'm not sure how to go about sharing the hyperviews, since they depend on the views themselves, which need to be unique to the 2 module implementations.
Currently, I've renamed some types, e.g.
AllTodos
=>MkTodosView
to facilitate my own understanding, but I'll revert this at the end.