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

Jump to bottom

feat: add initiatives page #107

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

feat: add initiatives page #107

wants to merge 13 commits into from

Conversation

georgimld
Copy link
Contributor

@georgimld georgimld commented Oct 15, 2024 •
edited
Loading

Todos:

  • add page for initiatives
  • chose a design -> used a ProjectCard similar to landing page
  • infinite scrolling
  • filtering based on search term -> graph ql query to look for the search string in all project properties
  • improve design -> avoided layout shifts, shift around the size of the cards and the descriptions
  • MobileNewsFeedFilter refactoring -> created a combined component MobileFilterDrawer, used for the initiatives page and the news page
  • Skeleton Refactoring -> create new Skeleton for ProjectCards
  • useEffect in filtering refactoring -> removed the useEffect from ProjectFilter.tsx

To check:

  • innitiatives page (check infinite scrolling, filtering, wide screen & mobile views)
  • check that the proejctCard looks good on the landing page (mobile & wide screen), i modified the card quite a bit
  • check the filtering also on the mobine view of the news page (updated some components)
  • check for layout shifts (except the known issue Layout shift of VisibleContributors in ProjectCards #115)

Closes #56

@georgimld georgimld self-assigned this Oct 15, 2024
@georgimld georgimld changed the title feat: add first draft of initiatives page feat: add initiatives page Oct 15, 2024
@pecirep pecirep mentioned this pull request Oct 29, 2024
Open
@georgimld georgimld marked this pull request as ready for review October 30, 2024 14:54
pecirep
pecirep requested changes Nov 5, 2024
View reviewed changes
app/app/projects/page.tsx Outdated Show resolved Hide resolved
app/components/common/project/ProjectCard.tsx Outdated Show resolved Hide resolved
app/components/common/skeletons/CardSkeleton.tsx Outdated Show resolved Hide resolved
app/components/common/MobileFilterDrawer.tsx Show resolved Hide resolved
app/components/common/MobileFilterDrawer.tsx Show resolved Hide resolved
app/components/common/MobileFilterDrawer.tsx Show resolved Hide resolved
pecirep
pecirep requested changes Nov 5, 2024
View reviewed changes
Copy link
Collaborator

@pecirep pecirep left a comment

Choose a reason for hiding this comment

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

The page content is not padded equally from the left and right side of the screen. This can probably be fixed by applying flex spacing styles on the container.

pecirep
pecirep requested changes Nov 5, 2024
View reviewed changes
Copy link
Collaborator

@pecirep pecirep left a comment

Choose a reason for hiding this comment

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

The Project Card skeleton doesn't match the Project Cards in layout, being wider and using different margins and corner radii. It doesn't cause a significant layout shift because the height is the same, but it would still be a less noticable transition when typing in searches if the positions of the elements inside matched.

This is the main reason I had all the skeleton components inside the ProjectCard. It did require checking loading state in a lot of places but it allowed me to match the style exactly without hassle and duplicating css. I think you are still correct in asking me to change that, but IMO we should approach the cards differently. If we have a more generic card component with well defined styles that we populate with slots, we can reuse it for both the project card and the project skeleton card without copy-pasting CSS and still only check loading state in one place.

pecirep
pecirep requested changes Nov 5, 2024
View reviewed changes
Copy link
Collaborator

@pecirep pecirep left a comment

Choose a reason for hiding this comment

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

I've noticed a lot of components using pixel margins, paddings and sizes and I think we should try to avoid that going forward and use EM and REM instead. https://www.w3schools.com/cssref/css_units.php
There's a few places where I have already done this for this PR and I can submit them either here on a separate branch, whichever you prefer

pecirep
pecirep requested changes Nov 5, 2024
View reviewed changes
Copy link
Collaborator

@pecirep pecirep left a comment

Choose a reason for hiding this comment

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

These changes seem to break the project carousel on the landing page in the mobile view, it's missing the gaps between the cards.

georgimld and others added 13 commits November 20, 2024 15:44
@georgimld @pecirep
feat: add first draft of initiatives page
f1f186e
@georgimld @pecirep
feat: implement initiatives page design, infinite scrolling and filte…
5072803 
…ring
@georgimld @pecirep
feat: extend graphql query to find search string in different project…
947dcd4 
… properties
@georgimld @pecirep
feat: improve ui, add small refactorings
25f3150
@georgimld @pecirep
feat: add skeleton for cards, improve ui
a2195d5
@georgimld @pecirep
fix: add ui fixes for the project card, remove use effect from projec…
801473a 
…tfilter
@georgimld @pecirep
refactor: combine filter drawers for projects and news feed pages
bae88cd
@georgimld @pecirep
fix: fix lint error in imports
c2e92ef
@pecirep
fix: minor layout simplifications and unit consolidations
72f3d1e 
Signed-off-by: pecirep <[email protected]>
@pecirep
refactor: use common compoent for cards with similar layout
74cf5c8 
use generic ContentCard as a template for ProjectCard and CardSkeleton

Signed-off-by: pecirep <[email protected]>
@pecirep
fix: improve landing page project carousel layout on different screen…
4ab756b 
… sizes

Signed-off-by: pecirep <[email protected]>
@pecirep
fix: improve (and simplify) projects page layout
a604ec5 
Signed-off-by: pecirep <[email protected]>
@pecirep
fix: simplify progress bar
278a94e 
use relative positioning instead of absolute and paint background of text
instead of calculating svg box size, resulting in more consistent padding and
reduced maximum width

Signed-off-by: pecirep <[email protected]>
georgimld
georgimld commented Nov 25, 2024
View reviewed changes
app/app/contexts/project-page-context.tsx
setHasMore(result.length >= pageSize);
} catch (error) {
errorMessage({ message: 'Error refetching projects. Please check your connection and try again.' });
console.error('Error refetching projects:', error);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

replace this with logger.error

georgimld
georgimld commented Nov 25, 2024
View reviewed changes
app/utils/requests/project/requests.ts
const projects = response.projects?.data.map(mapToBasicProject) ?? [];
return projects;
} catch (err) {
console.info(err);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

replace this with logger.error()

georgimld
georgimld commented Nov 25, 2024
View reviewed changes
Copy link
Contributor Author

@georgimld georgimld left a comment

Choose a reason for hiding this comment

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

Make sure to also fix the linting prpoblems, so the lint and build pass

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pecirep pecirep pecirep approved these changes

Assignees

@georgimld georgimld

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement separate page for initiatives
2 participants
@georgimld @pecirep