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: create mobile version of repo table #409

Merged
merged 20 commits into from
Sep 23, 2022
Merged

Conversation

OgDev-01
Copy link
Contributor

@OgDev-01 OgDev-01 commented Sep 21, 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 adds a mobile version of the RepositoriesTable component.
Details:
To achieve the expected result for this issue, I had to split the repo-row component into two (repo-row-mobile & repo-row-desktop).

Related Tickets & Documents

Fixes #410

Mobile & Desktop Screenshots/Recordings

Mobile
image

Opened table
image

Desktop
image

Tablet
image

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 Sep 21, 2022

βœ… Deploy Preview for design-insights ready!

Name Link
πŸ”¨ Latest commit 9c3795d
πŸ” Latest deploy log https://app.netlify.com/sites/design-insights/deploys/632dd312f24d9b000883d94d
😎 Deploy Preview https://deploy-preview-409--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 title feat: create mobile vertion of repo table feat: create mobile version of repo table Sep 21, 2022
@netlify
Copy link

netlify bot commented Sep 21, 2022

βœ… Deploy Preview for oss-insights ready!

Name Link
πŸ”¨ Latest commit 9c3795d
πŸ” Latest deploy log https://app.netlify.com/sites/oss-insights/deploys/632dd3126779c50009ee3c3a
😎 Deploy Preview https://deploy-preview-409--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.

@OgDev-01 OgDev-01 marked this pull request as ready for review September 22, 2022 22:10
@OgDev-01 OgDev-01 requested review from eryc-cc, 0-vortex, brandonroberts, bdougie and chadstewart and removed request for eryc-cc September 22, 2022 22:11
@OgDev-01 OgDev-01 added the needs review PRs that need review from core engineering team label Sep 22, 2022
Copy link
Contributor

@0-vortex 0-vortex left a comment

Choose a reason for hiding this comment

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

Unfortunately, this is not the way to build this. Since tailwind is a mobile first framework, the media query behaviour you needed here was unfortunately already implemented: https://tailwindcss.com/docs/responsive-design

The correct way to do the above requirements is to start with the mobile container, and, if none of the classes or elements match we hide the element from {size}-above - its desktop counterpart would get the equivalent of display none on lower-than-{size}.

You will have to remove all the media query related code, the duplicated components and then start from the mobile version and go up.

Here's an example of me doing specifically this for the hot opensauced header: open-sauced/hot@804de3e#diff-bf23bc8e35322afeda9bd76b71764c861a4b727f530f9330ef727fcaa5248357

@bdougie
Copy link
Member

bdougie commented Sep 22, 2022

Noticing on the desktop that the breakpoint could be sooner. Or we just remove the contributors sooner, since that seems to be the thing breaking.

Screen Shot 2022-09-22 at 3 26 22 PM

@OgDev-01
Copy link
Contributor Author

Unfortunately, this is not the way to build this. Since tailwind is a mobile first framework, the media query behaviour you needed here was unfortunately already implemented: https://tailwindcss.com/docs/responsive-design

The correct way to do the above requirements is to start with the mobile container, and, if none of the classes or elements match we hide the element from {size}-above - its desktop counterpart would get the equivalent of display none on lower-than-{size}.

You will have to remove all the media query related code, the duplicated components and then start from the mobile version and go up.

Here's an example of me doing specifically this for the hot opensauced header: open-sauced/hot@804de3e#diff-bf23bc8e35322afeda9bd76b71764c861a4b727f530f9330ef727fcaa5248357

Alright Ted, I'll work on these changes in the morning πŸ™πŸ™

@eryc-cc
Copy link
Contributor

eryc-cc commented Sep 22, 2022

Noticing on the desktop that the breakpoint could be sooner. Or we just remove the contributors sooner, since that seems to be the thing breaking.

Screen Shot 2022-09-22 at 3 26 22 PM

@sungoldtech @bdougie

I think for what Brian said:

  1. Remove contributors and "last 30 days"
  2. If the ones that are left are still a problem in smaller desktop viewports: add horizontal scrolling (remove it on mobile).

@OgDev-01
Copy link
Contributor Author

Unfortunately, this is not the way to build this. Since tailwind is a mobile first framework, the media query behaviour you needed here was unfortunately already implemented: https://tailwindcss.com/docs/responsive-design
The correct way to do the above requirements is to start with the mobile container, and, if none of the classes or elements match we hide the element from {size}-above - its desktop counterpart would get the equivalent of display none on lower-than-{size}.
You will have to remove all the media query related code, the duplicated components and then start from the mobile version and go up.
Here's an example of me doing specifically this for the hot opensauced header: open-sauced/hot@804de3e#diff-bf23bc8e35322afeda9bd76b71764c861a4b727f530f9330ef727fcaa5248357

Alright Ted, I'll work on these changes in the morning πŸ™πŸ™

Ping me if you need help or just have questions, you should not struggle with difficult things πŸ˜…

Also consider chatting on slack #frontend for this purpose! πŸ•

Alright Ted 😊

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

PR Compliance Checks

Thank you for your Pull Request! We have run several checks on this pull request in order to make sure it's suitable for merging into this project. The results are listed in the following section.

Watched Files

This pull request modifies specific files that require careful review by the maintainers.

Files Matched

  • npm-shrinkwrap.json
  • package.json

@OgDev-01 OgDev-01 added needs review PRs that need review from core engineering team and removed requested changes labels Sep 23, 2022
Copy link
Contributor

@eryc-cc eryc-cc left a comment

Choose a reason for hiding this comment

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

Looking better.

We should have min-width for each column so this doesn't happen:

Screen Shot 2022-09-23 at 11 41 38 AM

If it's too much, the horizontal scroll will take care of that.

@bdougie should this be a separate issue?

components/molecules/RepoRow/repo-row.tsx Outdated Show resolved Hide resolved
@bdougie
Copy link
Member

bdougie commented Sep 23, 2022

We can apply the quick fix. Plan on cutting a release today. So if it gets fixed in the next few hours let's do it here, otherwise we can make a new issue.

@OgDev-01
Copy link
Contributor Author

We can apply the quick fix. Plan on cutting a release today. So if it gets fixed in the next few hours let's do it here, otherwise we can make a new issue.

I'll apply this changes right away πŸ™Œ

@bdougie bdougie merged commit 9355968 into beta Sep 23, 2022
@bdougie bdougie deleted the mobile-repositories-tobile branch September 23, 2022 15:48
github-actions bot pushed a commit that referenced this pull request Sep 23, 2022
## [1.7.0-beta.7](v1.7.0-beta.6...v1.7.0-beta.7) (2022-09-23)

### πŸ• Features

* create mobile version of repo table ([#409](#409)) ([9355968](9355968))

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

* Update text styles on Scatter Chart card ([#429](#429)) ([ab24703](ab24703))
@github-actions
Copy link

πŸŽ‰ This PR is included in version 1.7.0-beta.7 πŸŽ‰

The release is available on GitHub release

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

github-actions bot pushed a commit that referenced this pull request Sep 23, 2022
## [1.7.0](v1.6.0...v1.7.0) (2022-09-23)

### 🎨 Styles

* added hover state style to footer links ([#420](#420)) ([3aa5f17](3aa5f17))
* change text font and icon size to xs(12px) ([#401](#401)) ([741288e](741288e))
* refactor show us button style to match design ([#419](#419)) ([b6c21b1](b6c21b1)), closes [#408](#408)

### πŸ› Bug Fixes

* add tooltip message on information icon hover ([#421](#421)) ([12ff851](12ff851)), closes [#366](#366)
* adds brandon to top5 ([8bb56cb](8bb56cb))
* correct beta branch checking beta gitsense api ([2c6f576](2c6f576))
* redirect from onboarding if already complete with custom hook ([#388](#388)) ([8600c76](8600c76)), closes [#387](#387)
* update PR overview calculation to use active PRs in last 30 days / total PRs ([#423](#423)) ([05b48ff](05b48ff)), closes [#418](#418)

### πŸ• Features

* add privacy and license terms ([#422](#422)) ([0dcc161](0dcc161)), closes [#377](#377)
* connect repositories page to API data and pagination ([#405](#405)) ([634de8e](634de8e)), closes [#320](#320) [#384](#384)
* create mobile version of repo table ([#409](#409)) ([9355968](9355968))

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

* check avatarURL for orange avatar background and update pill colors ([#424](#424)) ([c821cd5](c821cd5)), closes [#414](#414) [#416](#416)
* Polish the Dashboard to match designs ([#427](#427)) ([f919c27](f919c27))
* Polish the Reports Page to match the design ([#425](#425)) ([87a2bdc](87a2bdc))
* Update text styles on Scatter Chart card ([#429](#429)) ([ab24703](ab24703))
@github-actions
Copy link

πŸŽ‰ This PR is included in version 1.7.0 πŸŽ‰

The release is available on GitHub release

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

ElpisHelle added a commit to ElpisHelle/next.js-tailwindcss that referenced this pull request Aug 17, 2023
## [1.7.0-beta.7](open-sauced/app@v1.7.0-beta.6...v1.7.0-beta.7) (2022-09-23)

### πŸ• Features

* create mobile version of repo table ([#409](open-sauced/app#409)) ([9355968](open-sauced/app@9355968))

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

* Update text styles on Scatter Chart card ([#429](open-sauced/app#429)) ([ab24703](open-sauced/app@ab24703))
ElpisHelle added a commit to ElpisHelle/next.js-tailwindcss that referenced this pull request Aug 17, 2023
## [1.7.0](open-sauced/app@v1.6.0...v1.7.0) (2022-09-23)

### 🎨 Styles

* added hover state style to footer links ([#420](open-sauced/app#420)) ([3aa5f17](open-sauced/app@3aa5f17))
* change text font and icon size to xs(12px) ([#401](open-sauced/app#401)) ([741288e](open-sauced/app@741288e))
* refactor show us button style to match design ([#419](open-sauced/app#419)) ([b6c21b1](open-sauced/app@b6c21b1)), closes [#408](open-sauced/app#408)

### πŸ› Bug Fixes

* add tooltip message on information icon hover ([#421](open-sauced/app#421)) ([12ff851](open-sauced/app@12ff851)), closes [#366](open-sauced/app#366)
* adds brandon to top5 ([8bb56cb](open-sauced/app@8bb56cb))
* correct beta branch checking beta gitsense api ([2c6f576](open-sauced/app@2c6f576))
* redirect from onboarding if already complete with custom hook ([#388](open-sauced/app#388)) ([8600c76](open-sauced/app@8600c76)), closes [#387](open-sauced/app#387)
* update PR overview calculation to use active PRs in last 30 days / total PRs ([#423](open-sauced/app#423)) ([05b48ff](open-sauced/app@05b48ff)), closes [#418](open-sauced/app#418)

### πŸ• Features

* add privacy and license terms ([#422](open-sauced/app#422)) ([0dcc161](open-sauced/app@0dcc161)), closes [#377](open-sauced/app#377)
* connect repositories page to API data and pagination ([#405](open-sauced/app#405)) ([634de8e](open-sauced/app@634de8e)), closes [#320](open-sauced/app#320) [#384](open-sauced/app#384)
* create mobile version of repo table ([#409](open-sauced/app#409)) ([9355968](open-sauced/app@9355968))

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

* check avatarURL for orange avatar background and update pill colors ([#424](open-sauced/app#424)) ([c821cd5](open-sauced/app@c821cd5)), closes [#414](open-sauced/app#414) [#416](open-sauced/app#416)
* Polish the Dashboard to match designs ([#427](open-sauced/app#427)) ([f919c27](open-sauced/app@f919c27))
* Polish the Reports Page to match the design ([#425](open-sauced/app#425)) ([87a2bdc](open-sauced/app@87a2bdc))
* Update text styles on Scatter Chart card ([#429](open-sauced/app#429)) ([ab24703](open-sauced/app@ab24703))
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 released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature: We need a mobile version of the repositories table
4 participants