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: header navigation #24

Merged
merged 6 commits into from
Feb 18, 2024
Merged

fix: header navigation #24

merged 6 commits into from
Feb 18, 2024

Conversation

chraebsli
Copy link
Owner

  • fix: header navigation
  • remove percentage in skills
  • fix & cleanup: timeline

@chraebsli chraebsli linked an issue Feb 18, 2024 that may be closed by this pull request
4 tasks
Copy link

vercel bot commented Feb 18, 2024

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

Name Status Preview Updated (UTC)
portfolio ✅ Ready (Inspect) Visit Preview Feb 18, 2024 2:39pm

@chraebsli chraebsli merged commit 0f85db6 into main Feb 18, 2024
7 checks passed
@chraebsli chraebsli deleted the #6-fix-header-new branch February 18, 2024 14:41
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

PR Type: Enhancement

PR Summary: This pull request introduces several enhancements and fixes to the application. It focuses on improving the header navigation by integrating react-scroll for smoother navigation and updating the navigation links to use keys from the Sections type directly. Additionally, it removes the display of percentage values in the skills component and makes adjustments to the timeline component to be more responsive based on the device's screen size. The PR also includes minor adjustments such as adding a disabled text color to the theme and reintroducing a visual separator in the Section component.

Decision: Comment

📝 Type: 'Enhancement' - not supported yet.
  • Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
  • Unsupported files: the diff contains files that Sourcery does not currently support during reviews.

General suggestions:

  • Ensure that the use of @ts-ignore for importing react-scroll is absolutely necessary. If possible, define the types or find an alternative solution that does not suppress TypeScript's type checking.
  • Review the visual and functional changes in the header navigation to ensure they align with the intended user experience and design specifications.
  • Confirm that the removal of percentage display in the skills component and the responsive adjustments in the timeline component enhance the user interface without reducing the information quality or accessibility.
  • Consider the impact of theme changes, such as the addition of a disabled text color, on the overall consistency and appearance of the application.
src/components/common/Header.tsx: The code change introduces unnecessary complexity and potential maintainability issues by adding a custom `NavLink` component and bypassing TypeScript checks, suggesting a more integrated approach could simplify the implementation.

While the new features introduced in this change, such as smooth scrolling and the custom NavLink component, add interesting functionality, they also significantly increase the complexity of the code. Here are a few points to consider:

  1. The use of // @ts-ignore to bypass TypeScript's type checking can lead to potential issues down the line. It's generally best to address the type errors directly to maintain the robustness that TypeScript offers.

  2. The introduction of the custom NavLink component, while adding reusability, also adds another layer of abstraction. This increases the overall codebase and complexity, especially when the added functionality (like smooth scrolling) could potentially be integrated more directly with existing components.

  3. The modifications to the sections structure and the embedded styling and behavior in the NavLink component mix concerns that could be kept separate. This can make the code harder to maintain and understand.

  4. The added properties (spy, smooth, offset, duration) in the Link component from "react-scroll" are valuable for the smooth scrolling feature but also increase the cognitive load required to understand the component's behavior.

A simpler approach might involve enhancing the existing Nav.Link components directly with the necessary functionality, minimizing changes to the overall architecture. This would keep the codebase cleaner and more maintainable, focusing on integrating new features with minimal alterations to the existing structure.

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@@ -5,14 +5,16 @@ import { LanguageSwitcher } from "./LanguageSwitcher";
import { useTranslation } from "react-i18next";
import { Image } from "./Text";
import { Sections } from "../../type/page";
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
import { Link } from "react-scroll";
Copy link

Choose a reason for hiding this comment

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

issue (llm): Using @ts-ignore to bypass TypeScript checks can lead to runtime errors if the types change in future versions of react-scroll. Consider defining the types or using a different approach that doesn't suppress type checking.

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.

Fix Header
1 participant