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

Feature/cgd 35 #14

Merged
merged 18 commits into from
Sep 14, 2023
Merged

Feature/cgd 35 #14

merged 18 commits into from
Sep 14, 2023

Conversation

Dan-Y-Ko
Copy link
Contributor

@Dan-Y-Ko Dan-Y-Ko commented Sep 9, 2023

Only added banner to ideation page. Other pages should be the same for the most part, however the layout needs to be changed a bit to make it look good with this banner. That will be taken care of in another pr after the side nav is finished.

Removed docker and backend related things.

@vercel
Copy link

vercel bot commented Sep 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
chingu-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 14, 2023 7:25pm

@Dan-Y-Ko Dan-Y-Ko requested a review from marktlinn September 9, 2023 00:31
@Dan-Y-Ko
Copy link
Contributor Author

Dan-Y-Ko commented Sep 9, 2023

#15 (comment) for issue

Copy link
Contributor

@marktlinn marktlinn left a comment

Choose a reason for hiding this comment

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

The code looks good to me. I don't know if it's only on my machine, but the image for the banner section looks a little squashed and maybe a little blury.

@Dan-Y-Ko
Copy link
Contributor Author

Dan-Y-Ko commented Sep 11, 2023

The code looks good to me. I don't know if it's only on my machine, but the image for the banner section looks a little squashed and maybe a little blury.

Could you show me what it looks like? I don't think any of us have been too concerned about how it looks on different screen sizes and I haven't been addressing yet because I would like the side nav finished so we have more to work with regarding how the components are laid out and screen sizes. I'm not sure about the blurry thing though (maybe related)?

@marktlinn
Copy link
Contributor

The code looks good to me. I don't know if it's only on my machine, but the image for the banner section looks a little squashed and maybe a little blury.

Could you show me what it looks like? I don't think any of us have been too concerned about how it looks on different screen sizes and I haven't been addressing yet because I would like the side nav finished so we have more to work with regarding how the components are laid out and screen sizes. I'm not sure about the blurry thing though (maybe related)?

OK, no worries, I wasn't testing it at a small break point, just fullscreen on a laptop, the image just looked like it wasn't very good quality, but I don't think it's a big deal at this stage, as it can always be changed out for a higher quality version later.

@Dan-Y-Ko Dan-Y-Ko marked this pull request as ready for review September 12, 2023 17:24
@Dan-Y-Ko Dan-Y-Ko requested a review from marktlinn September 12, 2023 17:24
@Dan-Y-Ko
Copy link
Contributor Author

Dan-Y-Ko commented Sep 14, 2023

Ok so I added quite a bit to this pr.

Summary:

  • Added eslint rule to disable relative imports from a parent directory.
  • added pr template
  • re-organized the barrel exports

To explain how the barrel exports work:

  • Each folder will have its own barrel export file, named index.ts
  • The export in a barrel must be a named export.
  • When importing from same directory, prefer using the barrel file, unless there's a circular dependency (it is an issue in the store folder), this can potentially clean up the list of imports quite a lot.
  • When adding a subfolder to a parent's barrel file, just use export * from that folder. See root components folder for example.
  • When importing from a parent directory, always use absolute import and to the barrel file.

I also added this explanation to the confluence so other people can see it.

Copy link
Contributor

@marktlinn marktlinn left a comment

Choose a reason for hiding this comment

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

LGTM

@Dan-Y-Ko Dan-Y-Ko merged commit a286a98 into dev Sep 14, 2023
2 checks passed
@Dan-Y-Ko Dan-Y-Ko deleted the feature/cgd-35 branch September 14, 2023 19:28
Dan-Y-Ko added a commit that referenced this pull request Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants