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

Worked on the issue : Completely Disable Newsletter Signups #212 #216

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

Conversation

goutham03062001
Copy link

Hello, I've worked on the issue that you have assigned me "Completely Disable Newsletter Signups #212".

I made a few changes in two folders, i.e. DesktopNewsletter and MobileNewsletter folders.

In MobileNewsletter , I've simply replaced the built in alert with MaterialUI alert component. Here is the screenshot of that output.

image

You can see there is a nice alert is coming at the top of the webpage.

image

And the above one is for Desktop.. You can see that at the top.

You can suggest me, if you don't like this kind of alerts. I would like to work upon it.

Copy link
Collaborator

@ChristianBingman ChristianBingman left a comment

Choose a reason for hiding this comment

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

A couple of things:

  1. There are several changes here with no issues attached
  2. Adding such a library just for an alert like this is excessive
    Otherwise it looks good, but it is overkill for what we need.

@goutham03062001
Copy link
Author

goutham03062001 commented Apr 20, 2023

Ok, cool. I'll try come up without using any external packages.

Thank you for your feedback on my work.

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