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

Fix chevron toggle in Guides section of Sidebar #1567

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

Conversation

Utkarsh-123github
Copy link
Contributor

What kind of change does this PR introduce?
bugfix : This PR fixes the toggle of Chevron in guides section of sidebar

Issue Number:

Screenshots/videos:

Screen.Recording.2025-03-19.170828.mp4

Summary
This PR confirms that chevrons in guides section of sidebar gets perfectly toggled on click.

Does this PR introduce a breaking change?
No

Checklist

Please ensure the following tasks are completed before submitting this pull request.

@Utkarsh-123github Utkarsh-123github requested a review from a team as a code owner March 19, 2025 11:43
Copy link
Member

@DarhkVoyd DarhkVoyd left a comment

Choose a reason for hiding this comment

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

Hey @Utkarsh-123github, thanks for working on this and fixing the issue. This solution resolves the problem.

However, @benjagm, after reviewing the current code design and implementation, I believe that this isn't the correct design pattern to begin with. While this fix works for now, I think we should consider refactoring it in the future to follow a more scalable and maintainable design pattern.

@Utkarsh-123github
Copy link
Contributor Author

Utkarsh-123github commented Mar 23, 2025

Hey @DarhkVoyd,

I appreciate your insights on the current design pattern. For the GSoC 2025 season, there's a project focused on adapting a UI component library for our JSON Schema website. Implementing this project will help us structure and maintain components more effectively, making future refactoring much smoother and ensuring a more scalable and maintainable design.

@Utkarsh-123github
Copy link
Contributor Author

Utkarsh-123github commented Mar 23, 2025

So, I believe the issue you mentioned can be addressed within the scope of the project implementation.
Let me know your thoughts!

@DarhkVoyd
Copy link
Member

DarhkVoyd commented Mar 23, 2025

So, I believe the issue you mentioned can be addressed within the scope of the project implementation. Let me know your thoughts!

I don't see how migrating to a component library directly addresses this issue. If my concern wasn't clear, I meant that this implementation introduces redundant and duplicate code.

For example, the state structure and the multiple handleClick functions follow a repetitive pattern. Instead of managing separate boolean flags for each item, a more scalable approach could be using a single state variable that tracks the active section dynamically.

I misunderstood your comment. I see if we are planning to replace the sidebar too, that would fix this. 👍🏻

  const [active, setActive] = useState({
    getDocs: false,
    getStarted: false,
    getGuides: false,
    getReference: false,
    getSpecification: false,
  });
  useEffect(() => {
    const pathWtihoutFragment = extractPathWithoutFragment(router.asPath);
    const newActive = {
      getDocs: false,
      getStarted: false,
      getGuides: false,
      getReference: false,
      getSpecification: false,
    };
    if (getDocsPath.includes(pathWtihoutFragment)) {
      newActive.getDocs = true;
    } else if (getStartedPath.includes(pathWtihoutFragment)) {
      newActive.getStarted = true;
      setActive({ ...active, getStarted: true });
    } else if (getReferencePath.includes(pathWtihoutFragment)) {
      newActive.getReference = true;
    } else if (getSpecificationPath.includes(pathWtihoutFragment)) {
      newActive.getSpecification = true;
    } else if (getGuidesPath.includes(pathWtihoutFragment)) {
      newActive.getGuides = true;
    }

    setActive(newActive);
  }, [router.asPath]);

  const handleClickDoc = () => {
    setActive({
      getDocs: !active.getDocs,
      getStarted: false,
      getReference: false,
      getSpecification: false,
      getGuides: false,
    });
  };

  const handleClickGet = () => {
    setActive({
      getDocs: false,
      getStarted: !active.getStarted,
      getReference: false,
      getSpecification: false,
      getGuides: false,
    });
  };

  const handleClickReference = () => {
    setActive({
      getDocs: false,
      getStarted: false,
      getReference: !active.getReference,
      getSpecification: false,
      getGuides: false,
    });
  };

  const handleClickGuides = () => {
    setActive({
      getDocs: false,
      getStarted: false,
      getGuides: !active.getGuides,
      getReference: false,
      getSpecification: false,
    });
  };

  const handleClickSpec = () => {
    setActive({
      getDocs: false,
      getStarted: false,
      getGuides: false,
      getReference: false,
      getSpecification: !active.getSpecification,
    });
  };

  const rotate = active.getDocs ? 'rotate(180deg)' : 'rotate(0)';
  const rotateG = active.getStarted ? 'rotate(180deg)' : 'rotate(0)';
  const rotateR = active.getReference ? 'rotate(180deg)' : 'rotate(0)';
  const rotateSpec = active.getSpecification ? 'rotate(180deg)' : 'rotate(0)';
  const rotateGuides = active.getGuides ? 'rotate(180deg)' : 'rotate(0)';

@Utkarsh-123github
Copy link
Contributor Author

Utkarsh-123github commented Mar 23, 2025

@DarhkVoyd , the goal of the project is to adapt components from modern libraries. As part of this, we will be replacing necessary components such as the sidebar, buttons, accordion, and cards etc with those from the selected library while maintaining the current design and functionality and this will also avoid redundancy and duplication of code. As a result this update will also address the issue you raised.

@idanidan29
Copy link
Contributor

Hey @Utkarsh-123github, I noticed that the animation on the Guides section is still not behaving correctly. You should add the missing animation logic in line 614 to ensure consistency.

          className={classnames(
            'ml-6',
            'transition-all duration-500 ease-in-out',
            {
              'max-h-0 opacity-0 overflow-hidden': !active.getGuides,
              'max-h-80 opacity-100': active.getGuides,
            },
          )}

@Utkarsh-123github
Copy link
Contributor Author

@idanidan29 can you elaborate more like screenshot/video , because I didn't get what you are asking to do ??

@idanidan29
Copy link
Contributor

Sure @Utkarsh-123github,

Currently, there's a smooth animation transition when opening and closing all sections except for the Guides section. For example, you can see the changes in the transition in the video here:

Recording.2025-03-24.160032.mp4

The code shipment I added can help adjust this. Let me know if more elaboration is needed. I hope this helps

@Utkarsh-123github
Copy link
Contributor Author

Thanks @idanidan29 for noticing the minute details and bringing them to my attention.🚀🚀

@idanidan29
Copy link
Contributor

Thanks for your PR🚀🚀

@Utkarsh-123github
Copy link
Contributor Author

Hi @DhairyaMajmudar I hope you're doing well. When you have a moment, could you kindly review this PR at your convenience? I would really appreciate it. Thank you!

Copy link
Member

@DhairyaMajmudar DhairyaMajmudar left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @Utkarsh-123github it is working fine.

My thoughts also aligns with the comments of @DarhkVoyd

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.

🐛 Bug: Wrong toggle of arrows in Guide section
4 participants