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/tech stack functionality p2 #144

Merged
merged 28 commits into from
Jun 28, 2024
Merged

Conversation

MattRueter
Copy link
Contributor

Description

Added the following functionality to the tech-stack page of voyages.

  1. add an item
  2. delete an item
  3. edit an item
    Also made small adjustment to padding of card container.

Issue link

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@MattRueter MattRueter requested a review from Dan-Y-Ko as a code owner June 6, 2024 22:16
Copy link

vercel bot commented Jun 6, 2024

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 Jun 28, 2024 1:53am

Copy link
Collaborator

@Dan-Y-Ko Dan-Y-Ko left a comment

Choose a reason for hiding this comment

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

https://i.imgur.com/Uj3lK5a.png The border looks weird and cut off. Might be due to some kind of padding in the card, didn't look at the code but yea, that needs to be fixed. You can see example of what it should look like in either directory or features page. This is for the input when editing tech stack btw.

For the spinner, I'd like it in the button. You can also see example of that in directory or features page.

@JaneMoroz
Copy link
Collaborator

https://i.imgur.com/Uj3lK5a.png The border looks weird and cut off. Might be due to some kind of padding in the card, didn't look at the code but yea, that needs to be fixed. You can see example of what it should look like in either directory or features page. This is for the input when editing tech stack btw.

@MattRueter Can be fixed by adding overflow-visible to the parent container.

@MattRueter
Copy link
Contributor Author

https://i.imgur.com/Uj3lK5a.png The border looks weird and cut off. Might be due to some kind of padding in the card, didn't look at the code but yea, that needs to be fixed. You can see example of what it should look like in either directory or features page. This is for the input when editing tech stack btw.

@MattRueter Can be fixed by adding overflow-visible to the parent container.

@JaneMoroz I changed it by adjusting the padding in the parent. That seemed to fix it.

…gu-x/chingu-dashboard into feature/tech-stack-functionality-p2
Copy link
Collaborator

@Dan-Y-Ko Dan-Y-Ko left a comment

Choose a reason for hiding this comment

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

  1. Add some margin to the li in the tech stack card, the input is too squished to the scroll bar. I can't add it as a comment in this pr unfortunately. mr-2 is enough
  2. The input when editing should populate the data and not as a placeholder. You can see this behavior in directory and features page
  3. In the isediting logic, instead of repeating that twice with a truthy/falsy boolean check, just do a ternary.
  4. Your settingsmenu is nested inside the remove vote button. It shouldn't be nested like that since they are not directly related to each other and having it inside removebutton file doesn't make sense. I had trouble finding it myself until I did a search and found it that way.

@Stan-Stani
Copy link
Contributor

Stan-Stani commented Jun 20, 2024

https://jam.dev/c/13a5cd35-0161-4aaa-9161-c1b5693c47de

Settings modal is hidden when it overflows its container
image

Might be a future enhancement but the settings modal/popovers should probably be rendered in a portal or a div that's much higher in the DOM

But IDK how you'd make sure it's right by the ellipsis menu. Maybe instead just some logic that checks to see if it intersects with the edge of it's container and if so offset it up so it's fully visible?

Copy link
Collaborator

@Dan-Y-Ko Dan-Y-Ko left a comment

Choose a reason for hiding this comment

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

Just noticed, there needs to be validaton added on the input (both edit and new tech stack text inputs). Make sure to use Zod like it's being used everywhere else in the project. Simple required validation is good enough for now.

Everything else looks good though

@MattRueter
Copy link
Contributor Author

Just noticed, there needs to be validaton added on the input (both edit and new tech stack text inputs). Make sure to use Zod like it's being used everywhere else in the project. Simple required validation is good enough for now.

Will do. I had completely forgot about using Zod!

@MattRueter
Copy link
Contributor Author

https://jam.dev/c/13a5cd35-0161-4aaa-9161-c1b5693c47de

Settings modal is hidden when it overflows its container image

Yeah. It's the same (similar anyway) in features route.

Might be a future enhancement but the settings modal/popovers should probably be rendered in a portal or a div that's much higher in the DOM

But IDK how you'd make sure it's right by the ellipsis menu. Maybe instead just some logic that checks to see if it intersects with the edge of it's container and if so offset it up so it's fully visible?

Possibly using ref hook. @Dan-Y-Ko should design team be asked how they'd like the 'settings menu' to behave in cases of overflow?

@Dan-Y-Ko
Copy link
Collaborator

Dan-Y-Ko commented Jun 20, 2024

https://jam.dev/c/13a5cd35-0161-4aaa-9161-c1b5693c47de
Settings modal is hidden when it overflows its container image

Yeah. It's the same (similar anyway) in features route.

Might be a future enhancement but the settings modal/popovers should probably be rendered in a portal or a div that's much higher in the DOM
But IDK how you'd make sure it's right by the ellipsis menu. Maybe instead just some logic that checks to see if it intersects with the edge of it's container and if so offset it up so it's fully visible?

Possibly using ref hook. @Dan-Y-Ko should design team be asked how they'd like the 'settings menu' to behave in cases of overflow?

Yea I should probably ask what they want, will do that tomorrow. I'd imagine they would just want it so it auto adjusts to show the full container in view

@Dan-Y-Ko
Copy link
Collaborator

Fyi, as mentioned in the meeting, don't worry about the overflow issue for now

@Dan-Y-Ko Dan-Y-Ko merged commit 24026e4 into dev Jun 28, 2024
3 checks passed
@Dan-Y-Ko Dan-Y-Ko deleted the feature/tech-stack-functionality-p2 branch June 28, 2024 01:58
Dan-Y-Ko added a commit that referenced this pull request Jul 9, 2024
…y-p2

Feature/tech stack functionality p2
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.

4 participants