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

feat(neon_framework): Support custom backgrounds #943

Merged
merged 2 commits into from
Dec 22, 2023

Conversation

provokateurin
Copy link
Member

Closes #939

I'm pretty sure this whole feature but also the way I implemented it is going to be quite controversial, but please go ahead reviewing :)

@provokateurin provokateurin force-pushed the feat/neon/custom-background branch from 298abbd to 9c80331 Compare October 8, 2023 09:26
@provokateurin provokateurin force-pushed the feat/neon/custom-background branch from 9c80331 to 6700edc Compare October 19, 2023 19:54
Copy link
Member

@Leptopoda Leptopoda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked much at the code but it looks fine at a first glance.
But the design is just awe full and unacceptable.

I really struggle reading any text displayed so that's also a hard no go regarding #300.

@Leptopoda
Copy link
Member

I think we should not have a background behind the main app content but i quite like how the nextcloud web interface has the image behind the drawer (with sufficient contrast!!!).
The contrast is somewhat acceptable when both enabling dark mode and the oled override.
As the PR isn't rebased yet so I couldn't have a look at the dashboard app yet.

I think if the option is enabled we should only show the image behind the drawer. The dashboard app could then make its scaffold background transparent.
Each client developer should decide this on their own without a global rule.

@provokateurin provokateurin force-pushed the feat/neon/custom-background branch 2 times, most recently from 6ca0df8 to f73126b Compare October 29, 2023 10:42
@provokateurin
Copy link
Member Author

To implement this it would be great if there is a way to get cropped part of the background image. We don't want the image to repeat itself so all widgets would need to take the part of the image of their position as the background. I don't know if there is an easy way to achieve it, but using transparency like right now is definitely not good because it will break easily.

@provokateurin provokateurin force-pushed the feat/neon/custom-background branch from f73126b to 48e762c Compare November 2, 2023 10:32
@Leptopoda
Copy link
Member

We can still use the transparency approach but just not set it globally and only in the scaffold of the dashboard client

@provokateurin
Copy link
Member Author

The transparency approach is hard to implement and adds unnecessary code. "Punching a hole" would be way cleaner and easier to use everywhere.

@provokateurin provokateurin force-pushed the feat/neon/custom-background branch from 48e762c to f84cd25 Compare December 20, 2023 14:20
@provokateurin
Copy link
Member Author

For now I changed it so that it is a widget that can be used independently. Only used in the dashboard right now, but we can expand it later.

@provokateurin provokateurin changed the title feat(neon): Support custom backgrounds feat(neon_framework): Support custom backgrounds Dec 20, 2023
@provokateurin provokateurin merged commit 5618678 into main Dec 22, 2023
8 checks passed
@provokateurin provokateurin deleted the feat/neon/custom-background branch December 22, 2023 11:17
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.

Implement user selected background
2 participants