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

makes the totals row stay visible just over the punch button #1142

Merged
merged 9 commits into from
Feb 13, 2025

Conversation

Atomic-Germ
Copy link
Contributor

@Atomic-Germ Atomic-Germ commented Feb 5, 2025

Context / Background

While using the app, I find myself wanting to see the totals row when I’m higher up in the sheet.

What change is being introduced by this PR?

This just changes a little bit of the css to keep that row on screen.

How it is:
Screenshot 2025-02-05 at 3 40 53 PM

How it will be with this pr:
Screenshot 2025-02-05 at 3 45 15 PM

How will this be tested?

By scrolling!

@araujoarthur0
Copy link
Collaborator

Hey @Atomic-Germ, this is an interesting proposal. I say it might be a bit drastic to change visual elements by default and we could have a preference setting for this instead so it only applies to those who choose it. We already have a few preference values in the preferences page.
What do you think, @tupaschoal ?

@tupaschoal
Copy link
Collaborator

Actually I quite like it, I find myself scrolling to the bottom veery often when a month begins, I don't see how it would harm any one person usage, so if it works well, I'd make it default and non configurable.

Does this also touch the day view?

@Atomic-Germ
Copy link
Contributor Author

Looks like it does — if that isn’t wanted, there are a few ways around it. I can add some code here and there to make it behave normally in that window, oooor does it need to be displayed at all in day view?

@araujoarthur0
Copy link
Collaborator

Day view has its own bar with month balance, it's expected.
With your change it has a weird effect with more entries:

image

Copy link
Collaborator

@tupaschoal tupaschoal left a comment

Choose a reason for hiding this comment

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

I'm ok with having it in both day and month view, but it has to look correct on both.

@Atomic-Germ
Copy link
Contributor Author

I see what you're saying, I’ll work on a few different options for fixing that and see what looks best!

@Atomic-Germ
Copy link
Contributor Author

Atomic-Germ commented Feb 6, 2025

Screenshot 2025-02-06 at 3 40 54 PM Screenshot 2025-02-06 at 3 33 08 PM Screenshot 2025-02-06 at 3 40 16 PM Screenshot 2025-02-06 at 3 33 41 PM

See anything strange still? I’ll consider this a work in progress until we’re all good with it

EDIT: Added shots with the title in them

@araujoarthur0
Copy link
Collaborator

@Atomic-Germ I noticed it looks a bit rounder. Let's focus only on the proposed change here for stickiness and think about other styles later.
The scrollbar is appearing on the default view of the day calendar, which is not expected.
image

Text is also still appearing below and behind the button, which feels weird to me as originally the bar is part of the table and then it "jumps 1 dimension" above it. I know previously the button was transparent, but with the bar it's giving me a weird feeling. For example on the screenshot it looks like it's a table row with a purple border above and below.
image

Perhaps we should make the whole region around the punch button solid and no longer show the table below/behind it.

@Atomic-Germ
Copy link
Contributor Author

Atomic-Germ commented Feb 7, 2025

Screenshot 2025-02-07 at 3 11 08 PM Screenshot 2025-02-07 at 3 11 24 PM

And with the punch button disabled:
Screenshot 2025-02-08 at 10 51 47 AM

Copy link
Collaborator

@tupaschoal tupaschoal left a comment

Choose a reason for hiding this comment

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

I like how it looks now, what do you think @araujoarthur0 ?

If we stick to it, I'd just say we should delete the opacity property altogether, since the default is already 1.
I also wonder whether we should apply some effect to the background color of the disabled punch button, since it lost the opacity as disabled meaning.

Copy link

codecov bot commented Feb 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

see 1 file with indirect coverage changes

@Atomic-Germ
Copy link
Contributor Author

@tupaschoal How’s this? Needs to be tested for when it is enabled still though.
Screenshot 2025-02-08 at 12 33 34 PM
Screenshot 2025-02-08 at 12 34 42 PM

Copy link
Collaborator

@tupaschoal tupaschoal left a comment

Choose a reason for hiding this comment

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

Hmm, I don't like much the changing of the button to grey, I think it is too drastic. What if we use brightness filter for when it is disabled and when the user hovers it? I liked the result I played with (I'm not a CSS wizard, so there might be better ways)

Also, it seems like the image of the fingerprint is being cut by the footer. There was a small margin before but it is gone.

Needs to be tested for when it is enabled still though.

If you open the preferences and toggle the current day to be on then close it, the button will be enabled :)

Atomic-Germ and others added 4 commits February 9, 2025 09:32
Co-authored-by: Tulio Leao <tupaschoal@gmail.com>
Co-authored-by: Tulio Leao <tupaschoal@gmail.com>
Copy link
Collaborator

@tupaschoal tupaschoal left a comment

Choose a reason for hiding this comment

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

I like it too! Will wait for @araujoarthur0's take

@araujoarthur0
Copy link
Collaborator

Hover on enabled button is looking a little weird, it's like someone turned the flash on. I think the font is also leaking some of the light. I think changing the background color should be simpler than messing with the brightness as it's affecting everything inside it.

image

image

@Atomic-Germ
Copy link
Contributor Author

Looking further into it, this seems to be a perceptual thing. The contrast of the button is lowering by bringing the background color closer to white. I think leaving the hover transition for the enabled button out entirely.

@araujoarthur0
Copy link
Collaborator

Today it doesn't change on hover, right? It should be ok not to change that.

Copy link
Collaborator

@tupaschoal tupaschoal left a comment

Choose a reason for hiding this comment

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

I like there being some effect on hover of buttons, but I agree it should be considered separately. I'm mergint this as is, then, thanks for the patience @Atomic-Germ !

@tupaschoal tupaschoal merged commit 56977cd into TTLApp:main Feb 13, 2025
4 checks passed
@tupaschoal
Copy link
Collaborator

\changelog-update
Message: Enhancement [#1142]: Make balance row always visible above the punch button

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