open-sauced / intro

Empowering Your Open Source Journey: From First Contribution to Project Leadership
https://opensauced.pizza/learn
Other
544 stars 111 forks source link

Fixed padding between icon and text in UI #27

Closed Muniir1 closed 1 year ago

Muniir1 commented 1 year ago

Description

Fixed padding between icon and text in UI

Generated using OpenSauced.

What type of PR is this? (check all applicable)

Related Tickets & Documents

Fixes #20

Mobile & Desktop Screenshots/Recordings

the problem

old problemp

the result

new result

Added tests?

Added to documentation?

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

netlify[bot] commented 1 year ago

Deploy Preview for sauced-intro ready!

Name Link
Latest commit e8054bd146b666b29312a899dc0f80aa0e324424
Latest deploy log https://app.netlify.com/sites/sauced-intro/deploys/652318562139c600091521f3
Deploy Preview https://deploy-preview-27--sauced-intro.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Muniir1 commented 1 year ago

fixed no padding between icon and the text before the problem after the result

the problem was not only the introduction page also the other pages and i fixed all of them

here are different sizes before old problemp

after new result

Muniir1 commented 1 year ago

@bdougie please can you merge this?

dev-phantom commented 1 year ago

Please can you give this PR a descriptive title And also can you tag the issue this PR fixed You can use the (e.g Fixes #123) this helps to auto close the issue when this PR gets merged

Muniir1 commented 1 year ago

@dev-phantom thanks for your replay i made those changes, have a look

dev-phantom commented 1 year ago

@dev-phantom thanks for your replay i made those changes, have a look

Ok nice But it is "text" not "tex"

And I don't feel you need to add "tag" to the PR template as it already has "related tickets and documents"

Muniir1 commented 1 year ago

@dev-phantom i am really sorry, how about now!?

dev-phantom commented 1 year ago

@dev-phantom i am really sorry, how about now!?

No need to apologize The issue you fixed is #20 not #27 And you didn't change the "Tex" in the PR title

Muniir1 commented 1 year ago

@dev-phantom what about now!

bdougie commented 1 year ago

I'll also point out that we could potentially ship a fix upstream to the docsify project. Seems like a straight forward contribution @Muniir1

Muniir1 commented 1 year ago

@bdougie "Thank you for the insights! I understand the concern about using !important excessively and its potential impact on the custom UI. Considering the drawbacks, I agree that it would be best to explore other options and potentially build a custom solution to achieve the desired UI customizations. Additionally, I'll look into the possibility of contributing a fix or improvement to the 'docsify' project to address the issue more elegantly. Your input has been very helpful in guiding my approach. Thanks again!"

Muniir1 commented 1 year ago

@bdougie can you please check the changes that i just made and it works the same as the result before. and i do not use !important please have a look thanks

bdougie commented 1 year ago

@bdougie can you please check the changes that i just made and it works the same as the result before. and i do not use !important please have a look thanks

The changes are great and work. I question the approach manually adding the ids. What happens when we add a new section? Or a new language?

Does it make sense to modify the class-name instead?

Also, does it make more sense to fix here in the theme? https://github.com/docsifyjs/docsify/blob/develop/src/themes/vue.styl

NsdHSO commented 1 year ago

@Muniir1 why in the section of the description is checked the test checkbox ???