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

Added welcome screen #1374

Merged
merged 8 commits into from Oct 19, 2023
Merged

Conversation

Trial97
Copy link
Member

@Trial97 Trial97 commented Jul 16, 2023

fixes #827
20230716_174249

Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
@IntM4rco
Copy link

IntM4rco commented Jul 16, 2023

A little too rough, it should be improved aesthetically in my opinion, but it is still a nice addition.
In my opinion it would be better if the background is not so contrasting and the text is a bit smaller.
Also I would put the launcher logo at the top instead of that ( i ).
(Personal opinion of a regular user :P)

@TheKodeToad
Copy link
Member

Personally I'd just have "No instances :("
Maybe too vague though lol

@Trial97
Copy link
Member Author

Trial97 commented Jul 16, 2023

Yes I know is a crude implementation(is hard to paint with code).
The background stuff is because of the theme.
The size of the text and the text itself can be updated.
The current implementation reflects what I found in the mentioned issue.

@TheKodeToad
Copy link
Member

Personally I'd just have "No instances :(" Maybe too vague though lol

Oh, that's very ironic as this is literally based on my mockup 🤦‍♀️

@TheKodeToad
Copy link
Member

Idk. I think I find the icon weird 😵‍💫

Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
@Trial97
Copy link
Member Author

Trial97 commented Jul 16, 2023

Removed Icon and made text smaller

image

Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
@HyperSoop
Copy link
Contributor

HyperSoop commented Jul 21, 2023

Why is the background solid black? The contrast is definitely too much.

@TheKodeToad
Copy link
Member

It's the theme

@HyperSoop
Copy link
Contributor

This text doesn't actually tell one what they need to do to add an instance, so it's kind of useless

@Trial97
Copy link
Member Author

Trial97 commented Jul 21, 2023

This text doesn't actually tell one what they need to do to add an instance, so it's kind of useless

Right now I'm open to any suggestions, if you can provide a better language for that or a mockup of how the page should look like I would try to implement it

@HyperSoop
Copy link
Contributor

Maybe something like "Right click anywhere to create an instance.". The text should be only a little bit lighter than the background, just enough so to be visible. It should also be vertically centered.

@TheKodeToad
Copy link
Member

TheKodeToad commented Jul 21, 2023

This text doesn't actually tell one what they need to do to add an instance, so it's kind of useless

There's a fairly obvious button. Also lowering contrast is a bad idea imo.

@HyperSoop
Copy link
Contributor

This text is supposed to be a subtle hint, not a screaming reminder. In all software such a text is present, it doesn't contrast so much with the background it's on. Making it so big and so bright would be a definite UX issue.

@TheKodeToad
Copy link
Member

i don't think so. since it's only visible with no instances, it has no effect on useability.

image

if you take clion as an example, there's no dimming in the default dark mode

I feel like making the text harder to read on purpose is not a good idea lol

 into welcome_background

Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
@Trial97
Copy link
Member Author

Trial97 commented Jul 21, 2023

Also please note this screen will be visible only if the number of instances is zero.
And the contrast is from the theme I use.

@TheKodeToad
Copy link
Member

image
I just remembered this. maybe it can be improved in the future though

@getchoo getchoo added ui rework enhancement New feature or request labels Jul 25, 2023
@TheKodeToad TheKodeToad added the changelog:added A PR that appears under "Added" in the changelog label Jul 30, 2023
 into welcome_background

Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Copy link
Member

@DioEgizio DioEgizio left a comment

Choose a reason for hiding this comment

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

i think this is more useful.

also i think it's too big and the text should be smaller

Copy link
Member

@DioEgizio DioEgizio left a comment

Choose a reason for hiding this comment

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

i didn't add that to the review. oops

launcher/ui/instanceview/InstanceView.cpp Outdated Show resolved Hide resolved
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Copy link
Member

@DioEgizio DioEgizio left a comment

Choose a reason for hiding this comment

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

looks kinda bad but better than not having anything ig

Copy link
Member

@TayouVR TayouVR left a comment

Choose a reason for hiding this comment

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

considering how annoying it is to work within this code, LGTM

(whenever we revamp the instanceview fully (#30) maybe this could get changed to look nicer, be centered vertically, etc.)

@TayouVR TayouVR added this to the 8.0 milestone Oct 19, 2023
@TayouVR TayouVR merged commit b9c1dc7 into PrismLauncher:develop Oct 19, 2023
32 checks passed
@TheKodeToad
Copy link
Member

@RokeJulianLockhart can you comment instead of just downvoting?

@RokeJulianLockhart
Copy link

#1374 (comment)

@TheKodeToad, what do you refer to?

@TheKodeToad
Copy link
Member

this:
image

@RokeJulianLockhart
Copy link

RokeJulianLockhart commented Oct 20, 2023

#1374 (comment)

I know that. I ask which comment you refer to, @TheKodeToad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:added A PR that appears under "Added" in the changelog enhancement New feature or request ui rework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show a welcome background when there are no instances
8 participants