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

Implements page #559

Merged
merged 2 commits into from
Oct 8, 2024
Merged

Implements page #559

merged 2 commits into from
Oct 8, 2024

Conversation

Schwad
Copy link
Collaborator

@Schwad Schwad commented Oct 8, 2024

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 new SCARPE_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)

image

image

image

Checklist

  • Run tests locally

@Schwad Schwad self-assigned this Oct 8, 2024
@Schwad Schwad merged commit a0e0cc5 into main Oct 8, 2024
run: CI_RUN='true' bundle exec rake lacci_test
# Currently failing
# - name: Run Lacci tests
# run: CI_RUN='true' bundle exec rake lacci_test
Copy link
Collaborator

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?

Copy link
Collaborator Author

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

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.

Copy link
Collaborator Author

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

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?

Copy link
Collaborator

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.

Copy link
Collaborator Author

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"

Copy link
Collaborator

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.

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.

2 participants