Updating scaffolding to create default.css #254

ralfiannor · 2022-08-20T15:55:10Z

Implement #253

CLAassistant · 2022-08-20T15:55:15Z

All committers have signed the CLA.

matthewmueller · 2022-08-20T18:12:06Z

Thanks for the PR @ralfiannor and for kicking this off! I don't want to remove the default.css without providing a solution to the goal. I've wrote my thoughts up in #253. Let me know if you want to drive one of those solutions forward or if you have any other ideas!

Update master

matthewmueller

Thanks for working on this! I'd just like us to clean a few things up and add a few more tests (maybe in framework/public/public_test.go) and then I think we're good to go!

framework/view/ssr/svelte.ts

framework/view/ssr/svelte.js

internal/cli/create/create_test.go

internal/embedded/embedded.go

Fix staticcheck issues and add staticcheck to ci (livebud#263)

matthewmueller

Just reviewed. This is getting closer @ralfiannor, thank you!

framework/controller/controller_test.go

matthewmueller · 2022-09-02T03:23:52Z

internal/embedded/embedded.go

+// EmptyCss reset the default css data
+func EmptyCss() []byte {
+	return []byte("/* No Default CSS Loaded */")
+}


It'd be better if the view didn't include the <link /> if a public/default.css isn't present since that would avoid a roundtrip for this CSS sheet.

Then we can also remove this function.

matthewmueller · 2022-09-02T03:25:20Z

internal/cli/cli.go

@@ -49,6 +49,7 @@ func (c *CLI) Run(ctx context.Context, args ...string) error {
 		cmd := create.New(cmd, c.in)
 		cli := cli.Command("create", "create a new app")
 		cli.Arg("dir").String(&cmd.Dir)
+		cli.Flag("css", "add a css").String(&cmd.Css).Default("normalize")


Let's remove this flag for now, I'm not ready to expose it just yet, especially since it's not really possible to select an alternative.

matthewmueller · 2022-09-02T03:27:30Z

framework/public/public_test.go

+	res, err = app.Get("/default.css")
+	is.NoErr(err)
+	is.Equal(200, res.Status())
+	is.Equal(res.Body().Bytes(), defaultCss2)


It's a bit hard to follow these changes because there are new conditions added to existing tests.

Let's try to only touch existing tests that need to be updated for the new behavior, then add new tests for the new behavior.

Reverted. No need to update on public.go just update on ssr.go

matthewmueller · 2022-09-02T03:29:17Z

framework/view/ssr/svelte.js

@@ -327,7 +327,7 @@ var defaultLayout = {
        <html>
          <head>
            <meta charset="utf-8"/>
-            <style>${defaultCSS}</style>
+            <link rel="stylesheet" href="default.css" />


Do you mind trying to feed the configuration through so you can remove this if public/default.css doesn't exist? Let me know if it's difficult and I'll give it a go in this PR.

Will try. I will add the checking file existing in ssr.go but will call our logic each page loaded. Is it okay ?

I think it should be done at compile time actually. Do you know how to do this?

matthewmueller · 2022-09-02T04:05:21Z

I just realized we should also do the same thing for public/favicon.ico: scaffold a default one, but then give users the option to override it or delete it.

update from remote branch

revert the header with chunked change overlay fs to bud fs

ralfiannor · 2022-09-21T18:23:00Z

I just realized we should also do the same thing for public/favicon.ico: scaffold a default one, but then give users the option to override it or delete it.

Can we do it on another PR?

matthewmueller · 2022-09-23T20:08:17Z

Thanks for continuing to push this forward @ralfiannor!

Do you happen to know the condition in which Go is adding/removing Transfer-Encoding: chunked?

Let's just remove the Transfer-Encoding: chunked in the view tests and then I think this PR is good to go!

ralfiannor · 2022-09-25T01:36:41Z

Do you happen to know the condition in which Go is adding/removing Transfer-Encoding: chunked?

Yes, it happen when we added the CSS into <style></style> and the chunked also added

matthewmueller · 2022-09-25T06:31:28Z

Thanks @ralfiannor! I just pulled it down and did some manual testing. Two issues popped up:

Welcome page no longer shows up when creating a new project
Renaming or removing the default.css requires you to restart the bud process

Adjusting the global svelteRuntime is a bit hacky. If you're up for cleaning this up, a better solution would be to pass this information through the svelte.gotext template and then reference that information in the svelte.ts file. I'm okay with cleaning this up later if you're not sure how to do this.

Here's a diff with the 2 fixes above:

diff --git a/framework/app/app_test.go b/framework/app/app_test.go
index b5ba644..500a117 100644
--- a/framework/app/app_test.go
+++ b/framework/app/app_test.go
@@ -30,3 +30,26 @@ func TestWelcome(t *testing.T) {
 	is.NoErr(td.Exists("bud/app"))
 	is.NoErr(app.Close())
 }
+
+func TestWelcomeWithPublic(t *testing.T) {
+	is := is.New(t)
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	dir := t.TempDir()
+	td := testdir.New(dir)
+	td.Files["public/default.css"] = `* { box-sizing: border-box; }`
+	is.NoErr(td.Write(ctx))
+	cli := testcli.New(dir)
+	is.NoErr(td.NotExists("bud/app"))
+	app, err := cli.Start(ctx, "run")
+	is.NoErr(err)
+	res, err := app.Get("/")
+	is.NoErr(err)
+	is.Equal(res.Status(), 200)
+	is.In(res.Body().String(), "Hey Bud")
+	is.In(res.Body().String(), "Hey Bud") // should work multiple times
+	is.Equal(app.Stdout(), "")
+	is.Equal(app.Stderr(), "")
+	is.NoErr(td.Exists("bud/app"))
+	is.NoErr(app.Close())
+}
diff --git a/framework/view/ssr/ssr.go b/framework/view/ssr/ssr.go
index 7116bcd..8bab21c 100644
--- a/framework/view/ssr/ssr.go
+++ b/framework/view/ssr/ssr.go
@@ -50,9 +50,10 @@ type Compiler struct {
 
 func (c *Compiler) Compile(ctx context.Context, fsys budfs.FS) ([]byte, error) {
 	dir := c.module.Directory()
-
 	if existCss := vfs.Exist(fsys, "public/default.css"); nil == existCss {
 		svelteRuntime = strings.Replace(svelteRuntime, `<!-- default css -->`, `<link rel="stylesheet" href="default.css">`, 1)
+	} else {
+		svelteRuntime = strings.Replace(svelteRuntime, `<link rel="stylesheet" href="default.css">`, `<!-- default css -->`, 1)
 	}
 
 	result := esbuild.Build(esbuild.BuildOptions{
diff --git a/framework/web/loader.go b/framework/web/loader.go
index 5f0d149..e4695b8 100644
--- a/framework/web/loader.go
+++ b/framework/web/loader.go
@@ -52,7 +52,10 @@ func (l *loader) Load() (state *State, err error) {
 	l.imports.AddNamed("webrt", "github.com/livebud/bud/framework/web/webrt")
 	l.imports.AddNamed("router", "github.com/livebud/bud/package/router")
 	// Show the welcome page if we don't have controllers, views or public files
-	if len(exist) == 0 {
+	//
+	// TODO: welcome should be part of a default controller, then we can support
+	// a simple static file server without generating the welcome page.
+	if len(exist) == 0 || (len(exist) == 1 && exist["bud/internal/app/public/public.go"]) {
 		l.imports.AddNamed("welcome", "github.com/livebud/bud/framework/web/welcome")
 		state.ShowWelcome = true
 		state.Imports = l.imports.List()

matthewmueller · 2022-11-20T16:58:25Z

Any chance of wrapping this up @ralfiannor ?

matthewmueller marked this pull request as draft August 20, 2022 18:13

matthewmueller mentioned this pull request Aug 23, 2022

Default CSS in each page #253

Open

Merge pull request #1 from livebud/main

5cdb9da

Update master

ralfiannor changed the title ~~delete default css~~ Updating scaffolding to create default.css Aug 27, 2022

ralfiannor marked this pull request as ready for review August 27, 2022 21:47

matthewmueller reviewed Aug 29, 2022

View reviewed changes

framework/view/ssr/svelte.ts Outdated Show resolved Hide resolved

framework/view/ssr/svelte.js Outdated Show resolved Hide resolved

internal/cli/create/create_test.go Show resolved Hide resolved

internal/embedded/embedded.go Outdated Show resolved Hide resolved

Merge pull request #2 from livebud/main

17d64f0

Fix staticcheck issues and add staticcheck to ci (livebud#263)

matthewmueller requested changes Sep 2, 2022

View reviewed changes

ralfiannor and others added 9 commits September 19, 2022 00:48

Merge pull request #3 from livebud/main

2378cbf

update from remote branch

delete default css

7803102

add default css in render default layout

481da95

add normalize.css to embedded file

5b8253f

update scaffolding to add public/default.css

be8ad19

Change func name for clearly in embedded

6091965

add test case for default.css scaffolding

61afb45

remove emptyCss

d35f4e1

linked default css to svelte runtime when added

f721555

revert the header with chunked change overlay fs to bud fs

ralfiannor force-pushed the delete-default-css branch from 1e32ae5 to f721555 Compare September 21, 2022 16:33

ralfiannor added 2 commits September 22, 2022 00:10

reverted cuz default css is not implemented to public.go

ec8fa79

remove css parameter

d16a93d

matthewmueller approved these changes Sep 23, 2022

View reviewed changes

remove chunked header in unit test

f3067ee

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating scaffolding to create default.css #254

Updating scaffolding to create default.css #254

ralfiannor commented Aug 20, 2022

CLAassistant commented Aug 20, 2022 •

edited

matthewmueller commented Aug 20, 2022 •

edited

matthewmueller left a comment

matthewmueller left a comment •

edited

matthewmueller Sep 2, 2022

ralfiannor Sep 21, 2022

matthewmueller Sep 2, 2022

ralfiannor Sep 21, 2022

matthewmueller Sep 2, 2022 •

edited

ralfiannor Sep 21, 2022

matthewmueller Sep 2, 2022

ralfiannor Sep 7, 2022

matthewmueller Sep 7, 2022 •

edited

matthewmueller commented Sep 2, 2022 •

edited

ralfiannor commented Sep 21, 2022

matthewmueller commented Sep 23, 2022 •

edited

ralfiannor commented Sep 25, 2022 •

edited

matthewmueller commented Sep 25, 2022 •

edited

matthewmueller commented Nov 20, 2022

Updating scaffolding to create default.css #254

Are you sure you want to change the base?

Updating scaffolding to create default.css #254

Conversation

ralfiannor commented Aug 20, 2022

CLAassistant commented Aug 20, 2022 • edited

matthewmueller commented Aug 20, 2022 • edited

matthewmueller left a comment

Choose a reason for hiding this comment

matthewmueller left a comment • edited

Choose a reason for hiding this comment

matthewmueller Sep 2, 2022

Choose a reason for hiding this comment

ralfiannor Sep 21, 2022

Choose a reason for hiding this comment

matthewmueller Sep 2, 2022

Choose a reason for hiding this comment

ralfiannor Sep 21, 2022

Choose a reason for hiding this comment

matthewmueller Sep 2, 2022 • edited

Choose a reason for hiding this comment

ralfiannor Sep 21, 2022

Choose a reason for hiding this comment

matthewmueller Sep 2, 2022

Choose a reason for hiding this comment

ralfiannor Sep 7, 2022

Choose a reason for hiding this comment

matthewmueller Sep 7, 2022 • edited

Choose a reason for hiding this comment

matthewmueller commented Sep 2, 2022 • edited

ralfiannor commented Sep 21, 2022

matthewmueller commented Sep 23, 2022 • edited

ralfiannor commented Sep 25, 2022 • edited

matthewmueller commented Sep 25, 2022 • edited

matthewmueller commented Nov 20, 2022

CLAassistant commented Aug 20, 2022 •

edited

matthewmueller commented Aug 20, 2022 •

edited

matthewmueller left a comment •

edited

matthewmueller Sep 2, 2022 •

edited

matthewmueller Sep 7, 2022 •

edited

matthewmueller commented Sep 2, 2022 •

edited

matthewmueller commented Sep 23, 2022 •

edited

ralfiannor commented Sep 25, 2022 •

edited

matthewmueller commented Sep 25, 2022 •

edited