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

Refactoring of the Sidebar component and its styling #138

Merged
merged 4 commits into from
Oct 21, 2023

Conversation

AlexKomanov
Copy link
Contributor

Description

This PR contains several changes:

  • Fixing typo of the class sideber-menu to be sidebar-menu-toggle
  • Refactoring of the Sidebar component. A new folder named sidebar was added, that contains a component tsx file and a corresponding scss file.

The functionality was tested manually and it has the same behavior like before the PR.

This PR closes #133

Copy link
Member

@NoamGaash NoamGaash left a comment

Choose a reason for hiding this comment

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

nice job!
one more thing to go and we're there :)
thank you!

src/App.tsx Outdated
@@ -1,6 +1,7 @@
import React, { useCallback, useEffect } from 'react'
import 'antd/dist/antd.min.css'
import './App.scss'
import './pages/components/header/sidebar/sidebar.scss'
Copy link
Member

Choose a reason for hiding this comment

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

please move that import to the SideBar.tsx file

Copy link
Contributor Author

@AlexKomanov AlexKomanov Oct 21, 2023

Choose a reason for hiding this comment

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

@NoamGaash
Sorry. My mistake. The mentioned import of sidebar.scss is basically redundant inside App.tsx because it is already imported as part of SideBar.tsx . I removed the redundant one from the App.tsx.

Also, I removed unused import of React from 'react'.

image

Copy link
Member

Choose a reason for hiding this comment

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

@AlexKomanov thanks!
regarding React import, it is important:
image
you should import react to use tags in your tsx files

Copy link
Contributor Author

@AlexKomanov AlexKomanov Oct 21, 2023

Choose a reason for hiding this comment

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

Oh. Got it. Fixing my own created issue. :)

Copy link
Member

@NoamGaash NoamGaash 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 great. let's merge it once all checks are passing

@AlexKomanov
Copy link
Contributor Author

AlexKomanov commented Oct 21, 2023

There is a warning about my branch to be out of date. Should I update my branch? Or it can be merged with its current state?

@NoamGaash
Copy link
Member

it's completely fine - it can be merged :)
Mazal Tov! congrats for your first contribution

@NoamGaash NoamGaash merged commit fe69bb6 into hasadna:main Oct 21, 2023
6 checks passed
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.

typo: change sideber-menu css class to sidebar-menu-toggle
2 participants