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: Fix CSS inconsistency in Contributor Highlights for "Home" and "Following" tabs for /feed route #1783

Merged
merged 11 commits into from Oct 12, 2023

Conversation

5hraddha
Copy link
Contributor

@5hraddha 5hraddha commented Oct 2, 2023

Description

This PR fixes CSS inconsistency in Contributor Highlight Cards for "Home" and "Following" tabs for /feed route.

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

Related Tickets & Documents

Fixes #1778

Mobile & Desktop Screenshots/Recordings

Before the change:
https://github.com/open-sauced/app/assets/27571141/c5dbdbe5-7893-48d4-976a-c0d2450afa4c

272104773-c5dbdbe5-7893-48d4-976a-c0d2450afa4c.2.mov

After the change:
https://github.com/open-sauced/app/assets/27571141/e9e82d49-71df-4efd-83e3-672e6e3eb16f

272093777-e9e82d49-71df-4efd-83e3-672e6e3eb16f.mp4

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 Oct 2, 2023

βœ… Deploy Preview for oss-insights ready!

Built without sensitive environment variables

Name Link
πŸ”¨ Latest commit 84f6447
πŸ” Latest deploy log https://app.netlify.com/sites/oss-insights/deploys/6527306f0192370008fd79f1
😎 Deploy Preview https://deploy-preview-1783--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 configuration.

@netlify
Copy link

netlify bot commented Oct 2, 2023

βœ… Deploy Preview for design-insights ready!

Name Link
πŸ”¨ Latest commit 84f6447
πŸ” Latest deploy log https://app.netlify.com/sites/design-insights/deploys/6527306fabc3890008266cd2
😎 Deploy Preview https://deploy-preview-1783--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 configuration.

@nickytonline
Copy link
Member

Thanks for the PR @5hraddha! Mind adding a before and after screenshot or video in the description?

@5hraddha
Copy link
Contributor Author

5hraddha commented Oct 2, 2023

Mobile & Desktop Screenshots/Recordings

Thanks for the PR @5hraddha! Mind adding a before and after screenshot or video in the description?

Sure @nickytonline! I've added a video recording in Mobile & Desktop Screenshots/Recordings section after the change, but I'll do for the before the change too. Thanks!

@bdougie
Copy link
Member

bdougie commented Oct 2, 2023

I've added a video recording in Mobile & Desktop Screenshots/Recordings section after the change, but I'll do for the before the change too. Thanks!

I embedded them to save a click

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.

Looks good.

@5hraddha
Copy link
Contributor Author

5hraddha commented Oct 2, 2023

I've added a video recording in Mobile & Desktop Screenshots/Recordings section after the change, but I'll do for the before the change too. Thanks!

I embedded them to save a click

Thanks @bdougie!

Copy link
Member

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

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

Thanks for diving into this!! πŸ• πŸš€

Copy link
Member

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

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

(apologies for the re-review) Looks like, on diving deeper, this is still a problem with responsive, smaller screen sizes:

resized-home-following.mov

Notice when I make the screen abit smaller and the feed collapses in abit, the problem persists. On the other responsive screens (like for the smaller mobile view), it seems fine. Just the first resizing still seems broken.

Maybe there's an additional class we're missing?

@5hraddha
Copy link
Contributor Author

5hraddha commented Oct 3, 2023

(apologies for the re-review) Looks like, on diving deeper, this is still a problem with responsive, smaller screen sizes:

resized-home-following.mov
Notice when I make the screen abit smaller and the feed collapses in abit, the problem persists. On the other responsive screens (like for the smaller mobile view), it seems fine. Just the first resizing still seems broken.

Maybe there's an additional class we're missing?

Thanks for noticing that @jpmcb. I've fixed the glitch. Here is the recording for the reference. Please, let me know if that is suffice. Thanks!

opensauced-app-style-fix-v2.mp4

Copy link
Member

@OgDev-01 OgDev-01 left a comment

Choose a reason for hiding this comment

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

LGTM

@OgDev-01 OgDev-01 requested a review from jpmcb October 3, 2023 21:20
Copy link
Member

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

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

2023-10-03.15-36-58.mp4

It looks like there's still one view that doesn't have the same styling.

Notice the 3rd resizing in my screen capture still has a problem.

@5hraddha
Copy link
Contributor Author

5hraddha commented Oct 3, 2023

jpmcb

2023-10-03.15-36-58.mp4
It looks like there's still one view that doesn't have the same styling.

Notice the 3rd resizing in my screen capture still has a problem.

Hey @jpmcb, Could you please help me with the screen size where the CSS breaks?

@jpmcb
Copy link
Member

jpmcb commented Oct 3, 2023

Sure! If I go less than 1280px wide, it drops to a smaller size without the user profile column on the left:

Screenshot 2023-10-03 at 4 14 57 PM

Copy link
Member

@NsdHSO NsdHSO left a comment

Choose a reason for hiding this comment

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

Good to ship.

@5hraddha
Copy link
Contributor Author

5hraddha commented Oct 4, 2023

Am sorry that this is prolonging this far. But, it's bit weird @jpmcb. I could see different behaviours in different browsers. On Chrome, it's all good at screen width smaller than 1280px. Notice the sizes of the card on inspection. They are same. Attaching a recording for reference:

opensauce-google-test.mp4

On Firefox, I do see some negligible glitch. On inspection alone, I saw that there is a minute difference in the sizes of the card.

opensauce-firefox-test.mp4

Which browser are you checking on?

@nickytonline
Copy link
Member

Which browser are you checking on?

@jpmcb is using Firefox, so we should ensure it works well there as well.

@5hraddha
Copy link
Contributor Author

@nickytonline @jpmcb I figured out that there was a bug in tailwind.config.js while adding the additional smallest breakpoint - xs. As per the Tailwind documentation, if we want to add an additional small breakpoint, we can’t use extend because the small breakpoint would be added to the end of the breakpoint list, and breakpoints need to be sorted from smallest to largest in order to work as expected with a min-width breakpoint system. So, I have updated the config as suggested in the documentation.

Once that was done, my breakpoints were working as they should without the surprising behaviour. Then, I have added extra needed Tailwind classes to the component, to make sure they are rendered the same way in both the browsers.

Attaching the screen recording for reference:

Chrome

opensauced-app-style-fix-v3-chrome.mp4

Firefox

opensauced-app-style-fix-v3-firefox.mp4

I believe that should resolve the issue. Please, have a check and let me know if you find any other unusual CSS behaviour in this regard. Thank you!

@jpmcb
Copy link
Member

jpmcb commented Oct 10, 2023

Nice investigation on this!!!

I still see the same problem on the smaller view now only on Firefox:

Home feed (which seems to be slightly wider)

Screenshot 2023-10-10 at 1 53 25 PM

Following feed (which seems to be slightly narrower)

Screenshot 2023-10-10 at 1 53 10 PM

we can merge this in and try to roll forward a fix for the firefox edge case if people prefer

@5hraddha
Copy link
Contributor Author

Thanks for your patience, @jpmcb. Appreciate your keen eye on UI, too.

I just feel that there is still something missing that I'm not able to find at the moment. The UI should not be this quirky in Firefox. I'll keep looking into this and will raise an issue if I happen to find the problem.

Thanks again!

@brandonroberts brandonroberts merged commit ad08c16 into open-sauced:beta Oct 12, 2023
10 checks passed
open-sauced bot pushed a commit that referenced this pull request Oct 12, 2023
## [1.71.0-beta.2](v1.71.0-beta.1...v1.71.0-beta.2) (2023-10-12)

### πŸ› Bug Fixes

* Fix CSS inconsistency in Contributor Highlights for "Home" and "Following" tabs for `/feed` route ([#1783](#1783)) ([ad08c16](ad08c16))
* reset input state of delete list dialog box after closing the dialog and deleting the list ([#1877](#1877)) ([397570d](397570d))
@5hraddha 5hraddha deleted the shraddha/fix-css-consistenty branch October 16, 2023 18:59
open-sauced bot pushed a commit that referenced this pull request Oct 19, 2023
## [1.71.0](v1.70.0...v1.71.0) (2023-10-19)

### ⏩ Reverts

* clojure feature changes made directly to beta ([#1900](#1900)) ([7564245](7564245))

### πŸ• Features

* add clojure to dashboard interests ([7373865](7373865))
* add clojure to dashboard interests ([#1901](#1901)) ([e628041](e628041))
* add identifier to analytics ([#1887](#1887)) ([8169213](8169213))
* add keyboard binding for posting a highlight ([#1894](#1894)) ([346adff](346adff))
* Add OS detection hook and update search dialog ([#1760](#1760)) ([585a2aa](585a2aa))
* implemented usage of posthog.identify(identifier) ([30a4bc0](30a4bc0))
* issue support ([#1890](#1890)) ([a067789](a067789))
* show onboarding button on small screens ([#1803](#1803)) ([faf54bd](faf54bd))

### πŸ› Bug Fixes

* add check for listId to hooks ([#1869](#1869)) ([4bde212](4bde212))
* add responsiveness to profile detail ([#1892](#1892)) ([7466144](7466144))
* add target for the highlight link ([#1897](#1897)) ([d89d94b](d89d94b))
* Added isLoading condition to the title ([#1886](#1886)) ([03cd66d](03cd66d))
* adjust style for newsletter form so that it adjusts for smaller screens ([#1782](#1782)) ([9d97030](9d97030))
* attempt to clear any present service workers ([#1889](#1889)) ([e641223](e641223))
* can't update user profile bio passed 256 chars ([00a77be](00a77be))
* can't update user profile bio passed 256 chars ([#1898](#1898)) ([42c99c7](42c99c7))
* decreased delay time of tooltips ([3aef3c0](3aef3c0))
* decreased delay time of tooltips ([#1923](#1923)) ([fc50edf](fc50edf))
* Fix CSS inconsistency in Contributor Highlights for "Home" and "Following" tabs for `/feed` route ([#1783](#1783)) ([ad08c16](ad08c16))
* Fix horizontal padding /spacing on smaller screens for the Create New List form ([#1909](#1909)) ([e92a3d9](e92a3d9))
* hide secondary header in create insights page ([#1925](#1925)) ([a5245f7](a5245f7))
* list header misalignment now properly aligned ([#1941](#1941)) ([0224fa8](0224fa8))
* made active tab forecolor more prominent for hub navigation ([eeee241](eeee241))
* made active tab forecolor more prominent for hub navigation ([#1921](#1921)) ([fe997ed](fe997ed))
* match graph tooltip color with graph nodes ([#1930](#1930)) ([3f845d2](3f845d2))
* Modify Button Text ([#1942](#1942)) ([19a3484](19a3484))
* now legend labels in most active graph tooltip render properly ([#1927](#1927)) ([8a0ff79](8a0ff79))
* now treemap on list activity page appears on small screens ([c6ffb4a](c6ffb4a))
* now treemap on list activity page appears on small screens ([#1873](#1873)) ([92f9790](92f9790))
* refactored parameters for captureAnalytics to an object ([a2af672](a2af672))
* replace hash icon to arrow down for filter select ([#1825](#1825)) ([ee13255](ee13255))
* reset input state of delete list dialog box after closing the dialog and deleting the list ([#1877](#1877)) ([397570d](397570d))
* Unnecesary X-Scrollbar in tablist ([#1881](#1881)) ([9d72aeb](9d72aeb))
* update redirect paths to remove navigation errors ([#1888](#1888)) ([a2159d8](a2159d8))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Highlights "Home" and "Following" CSS not consistent
7 participants