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.

Sign up for GitHub

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: Feed page responsiveness #1490

Merged
merged 6 commits into from
Aug 7, 2023
Merged

fix: Feed page responsiveness #1490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d363f5f
Fix width to accommodate all screen size
RitaDee Aug 5, 2023
c07d2c1
Make UserCard Responsive in Flexbox Layout
RitaDee Aug 5, 2023
61907c3
Merge branch 'beta' into feed-page
RitaDee Aug 5, 2023
84654b7
Add container and px-2 md:px-16 classes to main element
RitaDee Aug 6, 2023
20644ee
Enhance the behaviour of the Tabs component by adding the 'grow' class
RitaDee Aug 6, 2023
c376ebf
Fix feed page responsiveness
RitaDee Aug 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions components/atoms/UserCard/user-card.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@ const UserCard = ({ username, name, meta, loading }: UserCardProps) => {
const avatarUrl = getAvatarByUsername(username);

return (
<div className="w-full pb-6 border rounded-lg bg-light-slate-1 border-zinc-200">
<div className="pb-6 border bg-light-slate-1 w-max rounded-xl border-zinc-200">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<div className="pb-6 border bg-light-slate-1 w-max rounded-xl border-zinc-200">
<div className="pb-6 border bg-light-slate-1 w-full rounded-lg border-zinc-200">

Return the full width for the user-card and the border-radius component.

Why though!?

  • The width w-full for size inheritance reason
  • The Border radius rounded-lg for uniformity reason, looking at all other sibling components/divs on the Right (Filter) Div and Left (userCard) div.

{loading ? (
<div className="flex items-center justify-center h-32 w-72">
<Spinner className="mt-6 " />
</div>
) : (
<div className="flex flex-col items-center gap-6 px-6 ">
<div className="flex flex-col items-center gap-6 px-9">
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm very curious to know why you thought to increase this padding!?

<div className="flex flex-col items-center gap-2 -mt-10">
<Image
className="border border-white rounded-full "
Expand Down
4 changes: 3 additions & 1 deletion layouts/profile.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ const ProfileLayout = ({ children }: { children: React.ReactNode }) => {
<div className="flex flex-col min-h-screen">
<TopNav />
<div className="page-container flex grow flex-col items-center">
<main className="flex pb-16 w-full flex-1 flex-col items-center bg-light-slate-2">{children}</main>
<main className="container px-2 flex md:px-16 pb-16 w-full flex-1 flex-col items-center bg-light-slate-2">
Copy link
Contributor

Choose a reason for hiding this comment

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

REGRESSION!!!

I see that the changes to the main occured in the ProfileLayout component, this will lead to Regression in other components where the ProfileLayout is integrated. See pages/component where regression occurs below with screenshots

image

image

This is due to the classes (container, px-2 and md:px-16) that we moved up into the component.

FIX!!

Suggested change
<main className="container px-2 flex md:px-16 pb-16 w-full flex-1 flex-col items-center bg-light-slate-2">
<main className="flex pb-16 w-full flex-1 flex-col items-center bg-light-slate-2">

We will have to remove the classes (container, px-2 and md:px-16), from there moving it back to its initial place in the child div.

{children}
</main>
</div>
<Footer />
</div>
Expand Down
13 changes: 5 additions & 8 deletions pages/feed/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,13 +126,10 @@ const Feeds: WithPageLayout<HighlightSSRProps> = (props: HighlightSSRProps) => {
image={ogImage}
twitterCard="summary_large_image"
/>
<div
className="container flex flex-col gap-16 px-2 pt-12 mx-auto md:px-16 lg:justify-end md:flex-row"
ref={topRef}
>
<div className="flex-col flex-1 hidden gap-6 mt-12 md:flex">
<div className="w-full gap-[2rem] justify-center flex flex-col md:gap-6 xl:gap-16 pt-12 md:flex-row" ref={topRef}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<div className="w-full gap-[2rem] justify-center flex flex-col md:gap-6 xl:gap-16 pt-12 md:flex-row" ref={topRef}>
<div className="w-full gap-[2rem] justify-center flex flex-col pt-12 md:flex-row" ref={topRef}>

The responsive/prefixed gap classes are not affecting anything at those breakpoints (i.e. xl and md), because the flex items the points are already reduced leaving the gap with nothing to space out.

So we can remove them.

<div className="flex-col flex-1 xl:flex hidden gap-6 mt-12">
Copy link
Contributor

@babblebey babblebey Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
<div className="flex-col flex-1 xl:flex hidden gap-6 mt-12">
<div className="w-[24%] flex-col flex-none xl:flex hidden gap-6 mt-12">

You are missing a width and flex-none from the Left (userCard) div.

With flex-none, the goal is to size the Left (userCard) div according to the width (that you will have to set to 24%), rendering it fully inflexible: so it neither shrinks nor grows in relation to its parent flex container.

In the absence of these we are have non equal width on the Right (Filter) Div and Left (userCard) div

image

{user && (
<div>
<div className="md:w-1/2 lg:w-1/3">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<div className="md:w-1/2 lg:w-1/3">
<div className="w-full">

This should only take full width of the parent div, not some fraction of it at breakpoints.

<UserCard
loading={loggedInUserLoading}
username={loggedInUser?.login as string}
Expand Down Expand Up @@ -222,7 +219,7 @@ const Feeds: WithPageLayout<HighlightSSRProps> = (props: HighlightSSRProps) => {
setActiveTab(value as activeTabType);
}}
defaultValue="home"
className="md:flex-[2] "
className="grow"
>
<TabsList className={clsx("justify-start w-full border-b", !user && "hidden")}>
<TabsTrigger
Expand Down Expand Up @@ -284,7 +281,7 @@ const Feeds: WithPageLayout<HighlightSSRProps> = (props: HighlightSSRProps) => {
<FollowingHighlightWrapper selectedFilter={selectedRepo} emojis={emojis} />
</TabsContent>
</Tabs>
<div className="hidden gap-6 mt-10 md:flex-1 md:flex md:flex-col">
<div className="md:hidden gap-6 mt-10 md:flex-1 lg:flex md:flex-col">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<div className="md:hidden gap-6 mt-10 md:flex-1 lg:flex md:flex-col">
<div className="md:hidden gap-6 mt-10 w-[30%] lg:w-[24%] flex-none lg:flex md:flex-col">

You are also missing a responsive width and flex-none from the Right (Filter) Div. for the same reason as Left (userCard) div.

...and

Suggested change
<div className="md:hidden gap-6 mt-10 md:flex-1 lg:flex md:flex-col">
<div className="hidden gap-6 mt-10 md:flex-1 lg:flex flex-col">

Tailwind uses a mobile-first breakpoint system.

This means that unprefixed classes (like hidden) take effect on all screen sizes, while prefixed one (like lg:flex) only take effect at the specified breakpoint (lg in this case) and above.

So Right Filter Div will be hidden normally on all screen sizes and it will only be in display again at lg (desktop) and above i.e. lg:flex.

So you needn't specify a responsive class of md:hidden since its gonna be hidden normally till lg.

{repoList && repoList.length > 0 && (
<HighlightsFilterCard
selectedFilter={selectedRepo}
Expand Down