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

fix: Add background to selectable tr table #252

Merged
merged 11 commits into from
Sep 6, 2022

Conversation

OgDev-01
Copy link
Contributor

@OgDev-01 OgDev-01 commented Aug 22, 2022

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • πŸ§‘β€πŸ’» Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert

Description

This PR fixes the background of selectable tr elements

Details:

  • This element was originally a table but was converted to a collection of divs and spans that act in a similar way.
  • There was already a function that selects a particular element and so functionality was added to change the background color depending on the checkbox attribute.
  • Replaced truncation function in table with CSS truncation (didn't even know CSS had truncation... What a time to be alive!)

Related Tickets & Documents

Fixes #183
Fixes #289

Mobile & Desktop Screenshots/Recordings

localhost_3000_hacktoberfest_repositories
localhost_3000_hacktoberfest_repositories (1)

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help

Added to documentation?

  • πŸ“œ README.md
  • πŸ““ docs.opensauced.pizza
  • πŸ• dev.to/opensauced
  • πŸ“• storybook
  • πŸ™… no documentation needed

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

@netlify
Copy link

netlify bot commented Aug 22, 2022

βœ… Deploy Preview for oss-insights ready!

Name Link
πŸ”¨ Latest commit 7046bba
πŸ” Latest deploy log https://app.netlify.com/sites/oss-insights/deploys/6314dfa11c525900086b0257
😎 Deploy Preview https://deploy-preview-252--oss-insights.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Aug 22, 2022

βœ… Deploy Preview for design-insights ready!

Name Link
πŸ”¨ Latest commit 7046bba
πŸ” Latest deploy log https://app.netlify.com/sites/design-insights/deploys/6314dfa1b02a910009a22cb4
😎 Deploy Preview https://deploy-preview-252--design-insights.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@OgDev-01 OgDev-01 changed the base branch from main to beta August 22, 2022 12:16
@OgDev-01 OgDev-01 added the help wanted Extra attention is needed label Aug 22, 2022
@OgDev-01
Copy link
Contributor Author

I've tried several tricks on overriding the default style of the select component, by none seems to work out... would need some help from @pixelsbyeryc or @chadstewart

@chadstewart
Copy link
Contributor

I've tried several tricks on overriding the default style of the select component, by none seems to work out... would need some help from @pixelsbyeryc or @chadstewart

Quick question, are you speaking of the components in the Select folder or the Selectable-Table component? Just wanted to clarify.

@eryc-cc
Copy link
Contributor

eryc-cc commented Aug 22, 2022

I've tried several tricks on overriding the default style of the select component, by none seems to work out... would need some help from @pixelsbyeryc or @chadstewart

Quick question, are you speaking of the components in the Select folder or the Selectable-Table component? Just wanted to clarify.

@chadstewart the Selectable-Table component, from this: #177 (comment)

Should we turn this <table> into a <div>, like @sungoldtech suggested on #183?

@chadstewart
Copy link
Contributor

I've tried several tricks on overriding the default style of the select component, by none seems to work out... would need some help from @pixelsbyeryc or @chadstewart

Quick question, are you speaking of the components in the Select folder or the Selectable-Table component? Just wanted to clarify.

@chadstewart the Selectable-Table component, from this: #177 (comment)

Should we turn this <table> into a <div>, like @sungoldtech suggested on #183?

If it's not too difficult of a conversion, I'd say this is probably the best thing we could do right now.

@OgDev-01
Copy link
Contributor Author

I've tried several tricks on overriding the default style of the select component, by none seems to work out... would need some help from @pixelsbyeryc or @chadstewart

Quick question, are you speaking of the components in the Select folder or the Selectable-Table component? Just wanted to clarify.

@chadstewart the Selectable-Table component, from this: #177 (comment)
Should we turn this <table> into a <div>, like @sungoldtech suggested on #183?

If it's not too difficult of a conversion, I'd say this is probably the best thing we could do right now.

Totally Agree with this approach πŸ™Œ

@OgDev-01
Copy link
Contributor Author

I've replaced the table with a div and also used span in a replacement for the tr. but I'm still trying to wrap my head around the logic of making the whole row have a background color when a checkbox is checked... @chadstewart I think I need assistance here πŸ™ŒπŸ™Œ

@chadstewart
Copy link
Contributor

I've replaced the table with a div and also used span in a replacement for the tr. but I'm still trying to wrap my head around the logic of making the whole row have a background color when a checkbox is checked... @chadstewart I think I need assistance here raised_handsraised_hands

Sorry for taking so long to get back to you @sungoldtech. Do you have a particular problem you are having or just the general understanding of the logic to complete this that you're having a problem with?

@eryc-cc
Copy link
Contributor

eryc-cc commented Aug 24, 2022

I've replaced the table with a div and also used span in a replacement for the tr. but I'm still trying to wrap my head around the logic of making the whole row have a background color when a checkbox is checked... @chadstewart I think I need assistance here πŸ™ŒπŸ™Œ

It's to visually indicate that the entire row is selected... When you have so much visual information on the screen, it can be hard to keep track of the checkbox only.

It also indicates where the clickable section is, so it's easier for people to know what will happen when they click something.

That's why it'd be good to have a hover state (light bg) + a selected state (slightly darker bg).

@OgDev-01
Copy link
Contributor Author

I've replaced the table with a div and also used span in a replacement for the tr. but I'm still trying to wrap my head around the logic of making the whole row have a background color when a checkbox is checked... @chadstewart I think I need assistance here raised_handsraised_hands

Sorry for taking so long to get back to you @sungoldtech. Do you have a particular problem you are having or just the general understanding of the logic to complete this that you're having a problem with?

It's the general understanding of the logic to complete the task based on the current implementation πŸ™Œ
Sorry for the late response @chadstewart

@chadstewart
Copy link
Contributor

I've replaced the table with a div and also used span in a replacement for the tr. but I'm still trying to wrap my head around the logic of making the whole row have a background color when a checkbox is checked... @chadstewart I think I need assistance here raised_handsraised_hands

Sorry for taking so long to get back to you @sungoldtech. Do you have a particular problem you are having or just the general understanding of the logic to complete this that you're having a problem with?

It's the general understanding of the logic to complete the task based on the current implementation raised_hands Sorry for the late response @chadstewart

No problem at all. It's async so we catch up whenever we can. If you'd like I can talk to you over a Google Meet to talk to you about what you need and unblock you. Let me know.

@chadstewart
Copy link
Contributor

Hey @sungoldtech. Sorry for doing this but I'm going to take over this PR and finish it so that you can focus on other activities.

Sorry about this.

@chadstewart chadstewart self-assigned this Sep 4, 2022
@OgDev-01
Copy link
Contributor Author

OgDev-01 commented Sep 4, 2022

Hey @sungoldtech. Sorry for doing this but I'm going to take over this PR and finish it so that you can focus on other activities.

Sorry about this.

It's all fine @chadstewart thanks for the help 🀝

@chadstewart chadstewart added needs review PRs that need review from core engineering team and removed help wanted Extra attention is needed labels Sep 4, 2022
@chadstewart chadstewart marked this pull request as ready for review September 4, 2022 15:38
@bdougie
Copy link
Member

bdougie commented Sep 4, 2022

Some noticeable inconsistencies.

Screen Shot 2022-09-04 at 10 48 16 AM

Screen Shot 2022-09-04 at 10 48 11 AM

Copy link
Member

@bdougie bdougie left a comment

Choose a reason for hiding this comment

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

LGTM

@bdougie bdougie merged commit 7d6ae16 into beta Sep 6, 2022
@bdougie bdougie deleted the add-background-to-selectable-tr-table branch September 6, 2022 16:50
github-actions bot pushed a commit that referenced this pull request Sep 6, 2022
## [1.4.0-beta.6](v1.4.0-beta.5...v1.4.0-beta.6) (2022-09-06)

### πŸ› Bug Fixes

* Add background to selectable tr table ([#252](#252)) ([7d6ae16](7d6ae16)), closes [#183](#183) [#289](#289)
* Contributor card overlap on smaller devices ([#298](#298)) ([777f6ba](777f6ba)), closes [#290](#290)
@github-actions
Copy link

github-actions bot commented Sep 6, 2022

πŸŽ‰ This PR is included in version 1.4.0-beta.6 πŸŽ‰

The release is available on GitHub release

Your semantic-release bot πŸ“¦πŸš€

github-actions bot pushed a commit that referenced this pull request Sep 6, 2022
## [1.4.0](v1.3.0...v1.4.0) (2022-09-06)

### πŸ§‘β€πŸ’» Code Refactoring

* Add types to useRepositoriesList hook ([#277](#277)) ([4f1a771](4f1a771))

### πŸ• Features

* Add download CSV funtionality to Reports page ([#286](#286)) ([ada79d3](ada79d3))
* **footer:** swap out footer text ([#296](#296)) ([737df5b](737df5b))
* Update Scatter Chart Legend ([#260](#260)) ([309ef62](309ef62))

### πŸ› Bug Fixes

* Add background to selectable tr table ([#252](#252)) ([7d6ae16](7d6ae16)), closes [#183](#183) [#289](#289)
* Contributor card overlap on smaller devices ([#298](#298)) ([777f6ba](777f6ba)), closes [#290](#290)
* correct netlify build supabase redirect url ([#283](#283)) ([0b2b80a](0b2b80a))
* Filter dropdown can only be closed by clicking on the filter button ([#299](#299)) ([bce1f8e](bce1f8e)), closes [#292](#292)
* Fixing issues with TopNav and Highlight Card ([#294](#294)) ([218f30d](218f30d)), closes [#287](#287) [#291](#291)
* grammar for header description ([#279](#279)) ([a019912](a019912))
ElpisHelle added a commit to ElpisHelle/next.js-tailwindcss that referenced this pull request Aug 17, 2023
## [1.4.0-beta.6](open-sauced/app@v1.4.0-beta.5...v1.4.0-beta.6) (2022-09-06)

### πŸ› Bug Fixes

* Add background to selectable tr table ([#252](open-sauced/app#252)) ([7d6ae16](open-sauced/app@7d6ae16)), closes [#183](open-sauced/app#183) [#289](open-sauced/app#289)
* Contributor card overlap on smaller devices ([#298](open-sauced/app#298)) ([777f6ba](open-sauced/app@777f6ba)), closes [#290](open-sauced/app#290)
ElpisHelle added a commit to ElpisHelle/next.js-tailwindcss that referenced this pull request Aug 17, 2023
## [1.4.0](open-sauced/app@v1.3.0...v1.4.0) (2022-09-06)

### πŸ§‘β€πŸ’» Code Refactoring

* Add types to useRepositoriesList hook ([#277](open-sauced/app#277)) ([4f1a771](open-sauced/app@4f1a771))

### πŸ• Features

* Add download CSV funtionality to Reports page ([#286](open-sauced/app#286)) ([ada79d3](open-sauced/app@ada79d3))
* **footer:** swap out footer text ([#296](open-sauced/app#296)) ([737df5b](open-sauced/app@737df5b))
* Update Scatter Chart Legend ([#260](open-sauced/app#260)) ([309ef62](open-sauced/app@309ef62))

### πŸ› Bug Fixes

* Add background to selectable tr table ([#252](open-sauced/app#252)) ([7d6ae16](open-sauced/app@7d6ae16)), closes [#183](open-sauced/app#183) [#289](open-sauced/app#289)
* Contributor card overlap on smaller devices ([#298](open-sauced/app#298)) ([777f6ba](open-sauced/app@777f6ba)), closes [#290](open-sauced/app#290)
* correct netlify build supabase redirect url ([#283](open-sauced/app#283)) ([0b2b80a](open-sauced/app@0b2b80a))
* Filter dropdown can only be closed by clicking on the filter button ([#299](open-sauced/app#299)) ([bce1f8e](open-sauced/app@bce1f8e)), closes [#292](open-sauced/app#292)
* Fixing issues with TopNav and Highlight Card ([#294](open-sauced/app#294)) ([218f30d](open-sauced/app@218f30d)), closes [#287](open-sauced/app#287) [#291](open-sauced/app#291)
* grammar for header description ([#279](open-sauced/app#279)) ([a019912](open-sauced/app@a019912))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review PRs that need review from core engineering team released on @beta
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Design breaks in mobile on Repositories Page [Bug] Add background to selectable <tr> table elements
4 participants