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

Add profile to example list #24

Merged
merged 1 commit into from Apr 30, 2021
Merged

Conversation

8BitJonny
Copy link
Contributor

Description

First of all: Thank you so much for such an amazing project. Just the other day when I was working on my github profile I wanted exactly this. Sadly that day I didn't come across this amazing project and so went through the hassle of recording a typing animation -> converting it to a gif -> uploading it to my repo -> and then finally linking it in my readme. This project makes it so much easier.

This PR adds my profile to the example list, now that I switched to use this project instead of my manually created gif. You can see two occurrences of the typing animation on my profile.

Lastly I want to ask if you (@DenverCoder1) already have plans to work on this margin-bottom that is present within the generated svg. The generated text is not vertically aligned and even when reducing the total height of the svg, first the text exits the frame at the top before the space at the bottom is reduced. This made proper positioning my readme a little more stressful and I would be interested in looking into vertically aligning the text or creating the feature to control the vertical spacing around the text.

Type of change

  • Bug fix (added a non-breaking change which fixes an issue)
  • New feature (added a non-breaking change which adds functionality)
  • Updated documentation (updated the readme, templates, or other repo files)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

No test because just documentation update

  • Ran tests with composer test
  • Added or updated test cases to test new features

Checklist:

  • I have checked to make sure no other pull requests are open for this issue
  • The code is properly formatted and is consistent with the existing code style
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

Screenshots

Because of the typing animation not really showing in a screenshot, I'm omitting a screenshot. Please have a look yourself to verify that I'm using this project in my readme: https://github.com/8BitJonny

Copy link
Owner

@DenverCoder1 DenverCoder1 left a comment

Choose a reason for hiding this comment

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

Looks good

@DenverCoder1
Copy link
Owner

Nice profile, and thanks for contributing! 🎉

Currently we have dominant-baseline="middle" when center is set to "true" and dominant-baseline="auto" when its not.

Maybe using dominant-baseline="middle" for both would make more sense so it's always centered vertically?

@8BitJonny
Copy link
Contributor Author

Interesting 🤔 I didn't notice that when playing around with it.

I mean, with dominant-baseline="auto" you almost have like an <h1>, <h2> kinda vibe because like them, the text doesn't have much margin-top but a lot more margin-bottom. And that makes sense. A great deal of people would use this text animation for headlines and then a general spacing to the following content makes sense.

But in my readme I already had that spacing and I think there are more of these use cases where you want to have equal margin on the top and bottom.

Maybe expose dominant-baseline="auto" as a query param and control it like valign=top or valign=center ?

@DenverCoder1
Copy link
Owner

Maybe expose dominant-baseline="auto" as a query param and control it like valign=top or valign=center ?

This sounds like a good idea, it's good to have a choice.

@DenverCoder1
Copy link
Owner

Feel free to open an issue

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.

None yet

2 participants