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

Added background to tool buttons to prevent button transparency fixes #753 #766

Merged
merged 2 commits into from Dec 26, 2021
Merged

Conversation

RaviAnand111
Copy link
Contributor

fixes #753
Changed btn-outline-secondary to btn-light with text formatter disabled and it is working fine.

@gitpod-io
Copy link

gitpod-io bot commented Dec 21, 2021

@RaviAnand111
Copy link
Contributor Author

@NARUDESIGNS , I forked and cloned again as I was unable to get to the previous version. So can you now review the pr.
ThankYou

@NARUDESIGNS
Copy link
Collaborator

I totally get you. I made the same mistake when I first contributed as well. My formatter (prettier) added too many changes and I used git revert <commit> to revert to an earlier commit and then made it right.
However I was going to ask, you removed "btn btn-outline-secondary" and added "btn btn-light", did this solve the problem?
Can you please share a screenshot or a shot video for us to see how those buttons look after you added your changes?
@RaviAnand111
cc @TildaDares

@RaviAnand111
Copy link
Contributor Author

publiclab.mp4

Here I have uploaded the video, you can check

@RaviAnand111
Copy link
Contributor Author

@NARUDESIGNS

@NARUDESIGNS
Copy link
Collaborator

@RaviAnand111 awesome, well done and thanks!
Please also rename the PR to be more descriptive about the issue it is fixing and make sure to include "fixes issue #753"
For example, maybe - "Added background to tool buttons to prevent button transparency fixes #753"
cc @TildaDares

@RaviAnand111 RaviAnand111 changed the title Disabled the formatter Added background to tool buttons to prevent button transparency fixes #753 Dec 22, 2021
@RaviAnand111
Copy link
Contributor Author

ThankYou @NARUDESIGNS, I have renamed the PR, you can see.

@NARUDESIGNS
Copy link
Collaborator

Awesome! @RaviAnand111
@TildaDares do you want to have a final look, please?

Copy link
Member

@TildaDares TildaDares left a comment

Choose a reason for hiding this comment

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

Great job everyone! Looks good to me🎉

@TildaDares
Copy link
Member

TildaDares commented Dec 22, 2021

Hi @RaviAnand111, usually when a new contributor opens a PR, a maintainer has to approve and then run the tests. I just did that so the 1 workflow awaiting approval text should be gone now.

Only users with push access to this repo can merge pull requests. You don't have to worry about the merging. You've done a great job!

I hope I was able to answer your questions well.

@RaviAnand111
Copy link
Contributor Author

Thanks, alot @TildaDares for explaining, I learned a lot of things about open source from this contribution and wish to learn more. I will keep contributing. ThankYou

@RaviAnand111
Copy link
Contributor Author

Hey @TildaDares @NARUDESIGNS , I just saw that PR run was failed, base-tests were not successful, Can you please tell me whats happening and what should I do to make it right.
ThankYou

@TildaDares
Copy link
Member

Hi @RaviAnand111, those tests aren’t working for now. @NARUDESIGNS is currently working on converting them from jasmine to jest.

@jywarren
Copy link
Member

This looks great. Thanks everyone!!!!!

@jywarren jywarren merged commit 313c3c0 into publiclab:main Dec 26, 2021
@welcome
Copy link

welcome bot commented Dec 26, 2021

Congrats on merging your first pull request! 🙌🎉⚡️
Your code will likely be published to PublicLab.org in the next few days. In the meantime, can you tell us your Twitter handle so we can thank you properly?
Do join our weekly check-in to share your this week goal and the awesome work you did 😃.
Reach out to someone else working on theirs on Public Lab's code welcome page. Thanks!
Now that you've completed this, you can help someone else take their first step!
See: Public Lab's coding community!

Help others take their first step

Now that you've merged your first pull request, you're the perfect person to help someone else out with this challenging first step. 🙌

https://code.publiclab.org

Try looking at this list of `first-timers-only` issues, and see if someone else is waiting for feedback, or even stuck! 😕

People often get stuck at the same steps, so you might be able to help someone get unstuck, or help lead them to some documentation that'd help. Reach out and be encouraging and friendly! 😄 🎉

Read about how to help support another newcomer here, or find other ways to offer mutual support here.

@RaviAnand111
Copy link
Contributor Author

yeah, my twitter handle is -
ravi_anand_dev

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.

Buttons has no background which makes it possible to see text behind it.
4 participants