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

Solution #686

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

PeaceOloruntoba
Copy link

@PeaceOloruntoba PeaceOloruntoba commented Jun 21, 2023

Describe your changes

On the Signup page for mobile screen. the navbar text are transparent, because they are green, so I created a new TS to set for mobile screen, and then I called it in the return function, if mobile screen text color should be white.

Issue ticket number and link

#685

Motivation and Context

I always want to contribute to open-source project, it makes me feel like a problem solver. I am motivated and driven to solve issues and problems that are made open source then I heard about the Igbo API being Open Source, I made my research on it and I discovered potential in it so, I decided to make my contributions to it.

(#685)

How Has This Been Tested?

Well, I haven't, but it can easily be reverted if its effect is bad.
I didn't run the test because my pc has a very low specs, running a program like this would require more resources than my pc has. I am a self-taught programmer, and an upcoming developer. I don't even have a full-time job yet, I am also a student, so I don't have a fixed workspace, or environment.
it has no effect in any other area, except if implemented in them, but for this pull request, it is implemented only in the Sub Menu component.

Screenshots (if appropriate):

That, I don't have.

I just did it coz I enjoy coding and developing, and I feel happy contributing to something great.

I think this should solve the issue of transparent navbar, and also don't forget to adjust the text-red-500 to the desired color, maybe light green 'text-green-200' or something
Copy link
Author

@PeaceOloruntoba PeaceOloruntoba left a comment

Choose a reason for hiding this comment

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

Someone should please approve this

@ijemmao
Copy link
Collaborator

ijemmao commented Jun 22, 2023

hey @PeaceOloruntoba thanks for opening up this PR! I just wanted to share a couple of notes:

  • your PR description is currently including comments (i.e. are comments in HTML) that shouldn't be included in the description - can you clean up your PR description
  • a more descriptive PR description that describes the changes that you made makes it easier for me to get a gist of the specific changes you've made rather than just knowing the expected outcome from the issue linked
  • we want to avoid casually merged code if you're able to run tests locally on your machine - since this code is used by more than a hundred developers 🙇🏾
  • ideally your title is more descriptive than just a single term that way this PR better represents the changes you've made

these aren't blocker comments, but I just wanted to give you a heads-up for future PRs!

Copy link
Collaborator

@ijemmao ijemmao left a comment

Choose a reason for hiding this comment

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

can you attach a screenshot to your PR description so we know what the new UI looks like?

Comment on lines 33 to 49
// Check if the viewport matches a mobile screen width
const mediaQuery = window.matchMedia('(max-width: 767px)');
const handleMediaQueryChange = (event) => {
setIsMobile(event.matches);
};

// Initial check
handleMediaQueryChange(mediaQuery);

// Listen for changes in viewport width
mediaQuery.addEventListener('change', handleMediaQueryChange);

// Clean up the event listener
return () => {
mediaQuery.removeEventListener('change', handleMediaQueryChange);
};
}, []);
Copy link
Collaborator

Choose a reason for hiding this comment

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

there's no need for this useEffect since we're able to apply styles based on viewport size via TailwindCSS classnames - you can remove this section

return (
<ul
className={`navbar ${transparent ? 'transparent-navbar' : ''}
${isVisible ? 'visible opacity-1' : 'hidden opacity-0'}
${isVisible ? '' : 'pointer-events-none'}
space-y-5 lg:space-y-0 lg:space-x-5 transition-all duration-100`}
space-y-5 lg:space-y-0 lg:space-x-5 transition-all duration-100
${isMobile ? 'text-red-500' : ''}`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of creating a new isMobile variable, you can rely on the TailwindCSS responsive classname paradigm to assign the text color based on the device size

Suggested change
${isMobile ? 'text-red-500' : ''}`}
text-red-500 md:text-gray // text-gray should be the default text color

Copy link
Collaborator

Choose a reason for hiding this comment

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

@PeaceOloruntoba bumping this comment

@ijemmao
Copy link
Collaborator

ijemmao commented Jun 22, 2023

Copy link
Collaborator

@ijemmao ijemmao left a comment

Choose a reason for hiding this comment

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

if you could update your PR description with more relevant details like describing the reasoning behind your changes and a screenshot, that would help me better understand your work - thanks 😊

Comment on lines +33 to +57
// Check if the viewport matches a mobile screen width
const mediaQuery = window.matchMedia('(max-width: 767px)');
const handleMediaQueryChange = (event) => {
setIsMobile(event.matches);
};

const [isMobile, setIsMobile] = useState(false);

useEffect(() => {
const mediaQuery = window.matchMedia('(max-width: 767px)');
const handleMediaQueryChange = (event) => {
setIsMobile(event.matches);
};

// Initial check
handleMediaQueryChange(mediaQuery);

// Listen for changes in viewport width
mediaQuery.addEventListener('change', handleMediaQueryChange);

// Clean up the event listener
return () => {
mediaQuery.removeEventListener('change', handleMediaQueryChange);
};
}, []); // Empty dependency array to run the effect only once
Copy link
Collaborator

Choose a reason for hiding this comment

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

none of this is necessary since you can just use responsive TailwindCSS classnames

Suggested change
// Check if the viewport matches a mobile screen width
const mediaQuery = window.matchMedia('(max-width: 767px)');
const handleMediaQueryChange = (event) => {
setIsMobile(event.matches);
};
const [isMobile, setIsMobile] = useState(false);
useEffect(() => {
const mediaQuery = window.matchMedia('(max-width: 767px)');
const handleMediaQueryChange = (event) => {
setIsMobile(event.matches);
};
// Initial check
handleMediaQueryChange(mediaQuery);
// Listen for changes in viewport width
mediaQuery.addEventListener('change', handleMediaQueryChange);
// Clean up the event listener
return () => {
mediaQuery.removeEventListener('change', handleMediaQueryChange);
};
}, []); // Empty dependency array to run the effect only once

Copy link
Collaborator

Choose a reason for hiding this comment

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

@PeaceOloruntoba please make this change - we don't need this extra code

@PeaceOloruntoba
Copy link
Author

Describe your changes

On the Signup page for mobile screen. the navbar text are transparent, because they are green, so I created a new TS to set for mobile screen, and then I called it in the return function, if mobile screen text color should be white.

Issue ticket number and link

#685

Motivation and Context

I always want to contribute to open source project, it makes me feel like a problem solver. I am motivated and driven to solve issues and problems that are made open source then I heard about the Igbo API being Open Source, I made my research on it and I discovered potential in it so, I decided to make my contributions to it.

[(https://github.com//issues/685)]

How Has This Been Tested?

Well I haven't, but it can easily be reverted if it's effect is bad.
I didn't run the test because my pc has a very low specs, running a program like this would require more resources than my pc has. I am a self-taught programmer, and an upcoming developer. I don't even have a full-time job yet, I am also a student, so I don't have a fixed workspace, or environment.
I has no effect in any other area, except if implemented in them, but for this pull request, it is implemented only in the Sub Menu component.

Screenshots (if appropriate):

That, I don't have.

I just did it coz I enjoy coding and developing, and I feel happy contributing to something great.

@ijemmao
Copy link
Collaborator

ijemmao commented Jun 26, 2023

@PeaceOloruntoba just bumping my last request for you to attach a screenshot of the change you've made since it's a visual change - I can't merge this PR without a screenshot. lmk how i can help!

Copy link
Collaborator

@ijemmao ijemmao left a comment

Choose a reason for hiding this comment

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

looks like you still need to address my two comments

Comment on lines +33 to +57
// Check if the viewport matches a mobile screen width
const mediaQuery = window.matchMedia('(max-width: 767px)');
const handleMediaQueryChange = (event) => {
setIsMobile(event.matches);
};

const [isMobile, setIsMobile] = useState(false);

useEffect(() => {
const mediaQuery = window.matchMedia('(max-width: 767px)');
const handleMediaQueryChange = (event) => {
setIsMobile(event.matches);
};

// Initial check
handleMediaQueryChange(mediaQuery);

// Listen for changes in viewport width
mediaQuery.addEventListener('change', handleMediaQueryChange);

// Clean up the event listener
return () => {
mediaQuery.removeEventListener('change', handleMediaQueryChange);
};
}, []); // Empty dependency array to run the effect only once
Copy link
Collaborator

Choose a reason for hiding this comment

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

@PeaceOloruntoba please make this change - we don't need this extra code

return (
<ul
className={`navbar ${transparent ? 'transparent-navbar' : ''}
${isVisible ? 'visible opacity-1' : 'hidden opacity-0'}
${isVisible ? '' : 'pointer-events-none'}
space-y-5 lg:space-y-0 lg:space-x-5 transition-all duration-100`}
space-y-5 lg:space-y-0 lg:space-x-5 transition-all duration-100
${isMobile ? 'text-red-500' : ''}`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@PeaceOloruntoba bumping this comment

@ijemmao
Copy link
Collaborator

ijemmao commented Jul 1, 2023

@PeaceOloruntoba are you still working on this PR?

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.

2 participants