scarpe-team / scarpe

Scarpe - shoes but running on webview
Other
162 stars 29 forks source link

Background as drawable #550

Open sapienfrom2000s opened 5 months ago

sapienfrom2000s commented 5 months ago

screen02 screen01

Description

Image(if needed, helps for a faster review)

Checklist

noahgibbs commented 4 months ago

When I run examples/stack/background.rb with the main branch, here's what I see:

Screenshot 2024-04-17 at 14 38 58

That looks reasonable to me.

I can't easily rebase this branch onto main -- there are some conflicts. But if I just run the code from this branch on the same example (not the same filename, the same code) then I get the same result with the branch. So that's a good start.

noahgibbs commented 4 months ago

This PR is based on this issue: https://github.com/scarpe-team/scarpe/issues/496

That issue gives example code that should change behaviour:

Shoes.app(width: 600, height: 50) do
  para "This is hidden under the background"
  background yellow
  para "This shows through on top"
end

When I run this with the code in the branch, I see both pieces of text. So that's not perfect - the background drawable should cover the first piece of text.

If I run with --debug, this is the HTML that gets sent (some backslashes added):

<div id=\\\"2\\\" style=\\\"display:flex;flex-direction:row;flex-wrap:wrap;align-content:flex-start;justify-content:flex-start;align-items:flex-start;width:100%;height:100%\\\"><div style=\\\"height:100%;width:100%;position:relative\\\"><p id=\\\"3\\\" style=\\\"font-size:12px\\\">This is hidden under the background</p><div id=\\\"4\\\" style=\\\"height:inherit;width:inherit;position:absolute;top:0;left:0;z-index:-99;box-sizing:border-box;background:rgba(255, 255, 0, 255);border-color:rgba(255, 255, 0, 255);border-width:1px;border-radius:px\\\"></div><p id=\\\"5\\\" style=\\\"font-size:12px\\\">This shows through on top</p><div id=\\\"root-fonts\\\"></div><div id=\\\"root-alerts\\\"> </div></div></div>

That's nearly right. But the z-index of -99 means that it is not covering the first piece of text, though it should. I think if that z index is removed it'll do what it should for that example.

I'm also seeing some oddity with the example in the branch because the background has been modified to add a curve. So it's harder for me to tell if it's doing the right thing.

noahgibbs commented 3 months ago

The change we were looking at when pairing:

index f2c0a18c..96737add 100644
--- a/scarpe-components/lib/scarpe/components/calzini.rb
+++ b/scarpe-components/lib/scarpe/components/calzini.rb
@@ -110,7 +110,7 @@ module Scarpe::Components::Calzini
   end

   def drawable_style(props)
-    styles = {}
+    styles = { :isolation => "isolate" }
     if props["hidden"]
       styles[:display] = "none"
     end
diff --git a/scarpe-components/lib/scarpe/components/calzini/background.rb b/scarpe-components/lib/scarpe/components/calzini/background.rb
index 1e74fc21..af927f39 100644
--- a/scarpe-components/lib/scarpe/components/calzini/background.rb
+++ b/scarpe-components/lib/scarpe/components/calzini/background.rb
@@ -14,7 +14,7 @@ module Scarpe::Components::Calzini
         position: :absolute,
         top: 0,
         left: 0,
-        'z-index': -99,
+        #'z-index': -99,
         'box-sizing': 'border-box',
       }

I think we need to use CSS isolation to do our own stacking context for everything since we're managing it all anyway.

noahgibbs commented 3 months ago

Still seeing a lot of test failures, presumably because of the "isolate" thing we talked about.

sapienfrom2000s commented 3 months ago

yes. I have regenerated the fixtures as discussed.

Pavan-Nambi commented 3 months ago

correct me if i am wrong i dont remember this exactly but - we need to generate fixtures only if we change current examples or add examples right?

for test failures we have to change expected outputs manually ?

noahgibbs commented 3 months ago

@Pavan-Nambi Exactly right, yeah. In this case he's adding a CSS attribute to a lot of tags, so both those things will need to change.