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

Reducing the width of "Punch" button to leave room for the scrollbars #455

Merged
merged 2 commits into from
Oct 13, 2020

Conversation

ccsCoder
Copy link
Contributor

Related issue

Closes #441

Context / Background

The reason for the gap is that the Scrollbar is Actually appearing on the body and it automatically reserves 10px for the scrollbar. Ideally, the scrollbar should appear on the container holding the table.
Leaving some room on the left and the right of the button looks like a good solution until we get to redoing the layout.

What change is being introduced by this PR?

  • Verified that the width was already maxed out .
  • The button was already center aligned, so I left 20px ---- button ---- 20px on both sides such that the scrollbar is clear of the button. Also reduced the width of the scrollbar such that it does not look overly big.
  • Direct consequence is that the look and feel of the punch button is definitely a bit different. Indirect consequence would be that we've identified some layout issues that we'll fix soon.

How will this be tested?

Will check if the button always reserves 20px from the left and from the right.

@ccsCoder
Copy link
Contributor Author

@thamara If I notice some code that can be refactored in a cleaner more terse way, should I go ahead with it ?
Do you want me to raise an Issue every time or can I simply raise a PR ?
Also, feel free to assign any other issue to me. "
P

@codecov
Copy link

codecov bot commented Oct 12, 2020

Codecov Report

Merging #455 into main will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #455      +/-   ##
==========================================
+ Coverage   63.95%   63.97%   +0.01%     
==========================================
  Files          28       28              
  Lines        2536     2537       +1     
  Branches      389      389              
==========================================
+ Hits         1622     1623       +1     
  Misses        806      806              
  Partials      108      108              
Impacted Files Coverage Δ
js/classes/FlexibleDayCalendar.js 70.73% <0.00%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa91728...ec0fcf2. Read the comment docs.

css/styles.css Outdated
Comment on lines 110 to 111
width: 5px;
height: 5px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a feeling 5px is too small... Not sure if it's just me.
@araujoarthur0 what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looked small on the other PR...
Is there a reason we are avoiding having the scrollbar only on the table part ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

As per @ccsCoder comment on the issue, this is not trivial right now. And from what I remember, I agree.

@ccsCoder, do you think we can keep it width at least 8 or 10 for the scroll bar, and take a little bit more pixels from the button?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In detail, if we want to have scrolling on the table, the right way to do it would be to have it on the tbody because we'd want to keep the Table Headers fixed.
The only way to do that (while still using <table and not divs) tag would be to have both <thead> and <tbody as display: block and have overflow set on them.

This works ( i've verified it ) but the "Punch button" overlaps the table. This is due to the fact that it's position is fixed which breaks it from the page layout flow. There are ways to get around that but the best way would be to change the markup in a way as follows ->

--- Title---
--- header ---
[[[ table ]]]. ---> expands to fill the available space.
-- footer---

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thamara How does this look ? Scrollbar is 8px wide here.

Screen Shot 2020-10-13 at 8 13 19 AM

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the great explanation.
Given that a lot of changes would be required and that the future of the fixed table is still under discussion, I guess we can go for the workaround right now.
But if you'd like to, you could open an issue for the flexible table. I suppose the same issue must be happening there, though it most likely is not a pure table element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

@thamara
Copy link
Collaborator

thamara commented Oct 13, 2020

@thamara If I notice some code that can be refactored in a cleaner more terse way, should I go ahead with it ? Do you want me to raise an Issue every time or can I simply raise a PR ?

Usually, I would say just raise a PR, but we got some big discussions regarding some components on the app (for example #401 ), and, to avoid having you doing some unnecessary work, I think it would be really good if you could just shout out refactoring changes you are seeing.
You can open a PR and we'll discuss in there, or feel free to join our discord server.

Also, feel free to assign any other issue to me.

Oh! Thank you a lot! During October the project receives lots of people and almost all the issues have someone working on them. You can check if there is any other issues open, and comment if you want to check it out. Of course, you can also propose new changes. :D

@thamara
Copy link
Collaborator

thamara commented Oct 13, 2020

Looks awesome!

@thamara
Copy link
Collaborator

thamara commented Oct 13, 2020

\changelog-update
Message: Enhancement: [#455] Small adjustments on the Punch button
User: ccsCoder

@thamara thamara merged commit 00f9aae into TTLApp:main Oct 13, 2020
@ccsCoder ccsCoder deleted the issue-441 branch October 13, 2020 04:01
thamara pushed a commit to jcombs0929/time-to-leave that referenced this pull request Oct 17, 2020
…TTLApp#455)

* Reducing the width of "Punch" button to leave room for the scrollbars

* Increasing scrollbar width as per review comments
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.

Punch button should overflow to the right
3 participants