-
Notifications
You must be signed in to change notification settings - Fork 29
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
Implements page
#559
Implements page
#559
Conversation
run: CI_RUN='true' bundle exec rake lacci_test | ||
# Currently failing | ||
# - name: Run Lacci tests | ||
# run: CI_RUN='true' bundle exec rake lacci_test |
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.
Hm. This turns off all Lacci tests?
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 filed an issue when I found that I was getting the same Laci failures on CI on this branch, locally on this branch, and locally on main.
Maybe this was too aggressive, but thought that could be the best approach to isolate that issue in a separate PR
Maybe it's not reproducible though? I think you have commented on this issue so let's revisit
I definitely do want these tests. But if they were going red on main I wanted to toggle them until resolved
ret = yield | ||
@current_app = old_cur_app | ||
ret | ||
old_app = Thread.current[:shoes_app] |
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.
Interesting. Were you doing something that needed this? This is going to change multithreaded behaviour... Which is maybe fine? This way could be better if you have multiple threads and you assume each thread will be running a different app. It will fail if multiple threads ever need to use the same app.
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 was a bit deep when I put this together. Let me revisit this code and either explain myself or revert. I'd mentioned in the discord but some of my contributions today were a bit aggressive which increases the risk that I shipped a change not pivotal to my feature.
with_slot(@document_root, &@app_code_body) | ||
with_slot(@document_root) do | ||
@content_container = flow(width: 1.0, height: 1.0) | ||
with_slot(@content_container, &@app_code_body) |
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 automatically makes a new flow around every app?
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.
Ah, okay. There's a new flow that then contains whatever the page is. Hrm. I should figure out if that messes with the formatting. I know with CSS stuff we had a devil of a time figuring out when adding a new container to the tree changed formatting. But this one may be fine.
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 was having bugs where certain content was getting painted over and over on button click. Part of resolving this, and I thought this made sense, was a silent container for each and every "page"
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.
Hm. Since you're clearing the content_container every time, I'd think it'd make more sense to clear the DocumentRoot. If you're getting paint errors with that method, we should figure out why and fix those. Also, can you skip the stack() around pages if you're doing this? It seems odd to me that pages stack vertically by default but everything else stacks horizontally. And I think you can just skip making the extra container.
Description
As I look to implement
url
, I am making a vision decision that new features may be extended. I give the rationale and process in a newSCARPE_FEATURES.md
doc. The TL;DR is we will continue to strive for backwards compatibility on Shoes.rb. But I want that to be a floor, not a ceiling. I want that to be a tailwind, not a headwind.I want to build desktop apps, and we've put enough work into this I think we're afforded our own taste and building as well.
This is a simpler version of "url", "page". You name and declare it in a block, and you can access it with "visit". It works very nicely for navigation. Unlike URL, it does not accept parameters. So it's handy if you quickly want pages.
Image(if needed, helps for a faster review)
Checklist