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

refactor-66: Disable other option when three categories is selected #69

Closed
wants to merge 3 commits into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Dec 3, 2023

Summary

Disable other option when three categories is selected

Description

To improve the current UX, disable the remaining categories when the user selects three categories while a new blog post is being created.

Current Behavior

Currently if a user selects more than three categories, they won't get any feedback until after clicking on the "Create Blog" button.This degrades the UX as they will have to unselect certain categories and try to click on the submit button again.

##Fixed Behavior

I propose that once the user selects three categories, other categories go into a disabled state.
If the user de-selects one of the three, all the categories will be shown as available again.

Issue(s) Addressed

Closes #66

Prerequisites

Copy link

vercel bot commented Dec 3, 2023

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

Name Status Preview Comments Updated (UTC)
wanderlust ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 3, 2023 2:47pm
wanderlust-backend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 3, 2023 2:47pm

@krishnaacharyaa
Copy link
Owner

krishnaacharyaa commented Dec 3, 2023

Hi @SanjeevKrSingh, Thank you for the quick PR, really appreciated. Just couple of things here to add,

  1. PR description needs to be properly edited (you can refer other merged PRs for this)
    a. The issue addressed section
    b. And check the box of "followed community guidelines?"
  2. And instead of toast message here, can we grey out rest of the categories and not let them click at all? That would be more appropriate here, otherwise it is still, they will get to know only after them clicking at the fourth category. Doesn't completely fix the issue.
  3. commit message doesn't have to have issue number, its only the PR title, kindly read the contributing guidelines once again for further clarification, so that we don't end up redoing the task :)

Copy link
Owner

@krishnaacharyaa krishnaacharyaa left a comment

Choose a reason for hiding this comment

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

Sadly, we will have to park this PR for now because it tries to make changes to a file which will be deleted in the PR #67, we'll take this up once this PR is merged, so that we don't conflict with the changes

: categoryProps(category, false)
className={`${isDisable && !formData.categories.includes(category) ? "cursor-not-allowed" : "cursor-pointer"}
${formData.categories.includes(category)
? categoryProps(category, true, isDisable ? true : false)
Copy link
Owner

Choose a reason for hiding this comment

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

Can you kindly optimize this even further, refactoring it to a new function which would simplify this process further.

Copy link
Author

Choose a reason for hiding this comment

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

Ok @krishnaacharyaa I will do that

switch (category) {
case 'Travel':
return `${commonProps} ${selected ? `bg-pink-500` : `bg-pink-100`}`;
return `${commonProps} ${selected ? `bg-pink-500` : disable ? `bg-slate-100 text-slate-400` : `bg-pink-100`}`;

Copy link
Owner

Choose a reason for hiding this comment

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

Okay, I guess we have to figure out a more optimized version of getting this done,
And also we cannot carry with this approach because of #67, in this PR this category-props is totally removed :)

Copy link
Author

Choose a reason for hiding this comment

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

Sir @krishnaacharyaa, can I remove category-props. and fixed it?

Copy link
Author

Choose a reason for hiding this comment

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

Sir @krishnaacharyaa Plz let me know whenver #67 is merged I will fixed the above comment and push it.

Copy link
Owner

@krishnaacharyaa krishnaacharyaa Dec 3, 2023

Choose a reason for hiding this comment

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

Sir @krishnaacharyaa, can I remove category-props. and fixed it?

Actually, we are introducing new component called category-pill if you see the PR, so the way we implement
this feature, would be slightly different.

Sir @krishnaacharyaa Plz let me know whenver #67 is merged I will fixed the above comment and push it.

Sure

@krishnaacharyaa krishnaacharyaa marked this pull request as draft December 3, 2023 14:54
@krishnaacharyaa
Copy link
Owner

@SanjeevKrSingh, Kindly rebase, or better would be create new fresh branch and you can work now the PR got merged :)

@chinmaykunkikar
Copy link
Collaborator

Hey @SanjeevKrSingh, thanks for working on this.

Just to make sure we get this working without making it complicated, I'll walk you through about modifying the components/category-pill.tsx for this to work after #67:

export default function CategoryPill({ category, selected = false }: CategoryPillProps) {

Add a prop here called disabled = false like selected = false (don't forget to add disabled: string type after line no. 6).
Then, introduce a const disabledColor: string = "bg-slate-100 text-slate-400" in the same file.
Then you can simply add disabled ? disabledColor : defaultColor to the twMerge() argument.

Ask me if you have any questions or if I overlooked some logic here :)

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.

[ENHANCEMENT] Disable other options if three categories are selected while creating a post
2 participants