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

Background as drawable #550

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

sapienfrom2000s
Copy link
Contributor

screen02
screen01

Description

Image(if needed, helps for a faster review)

Checklist

  • Run tests locally

@noahgibbs
Copy link
Collaborator

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
Copy link
Collaborator

This PR is based on this issue: #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
Copy link
Collaborator

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
Copy link
Collaborator

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

@sapienfrom2000s
Copy link
Contributor Author

yes. I have regenerated the fixtures as discussed.

@Pavan-Nambi
Copy link

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
Copy link
Collaborator

@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.

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.

3 participants