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

Fix bug #196 #197

Closed
wants to merge 9 commits into from
Closed

Fix bug #196 #197

wants to merge 9 commits into from

Conversation

kaushikpadmanaban
Copy link
Contributor

Fix banner responsivity and add side-scrolling

Bug discussed here

All overflowing elements now resize responsively to screen size

As discussed in #196, this commit solves banner responsivity issues by removing its max-width attribute. In a subsequent comment, it was discussed that the contributor tab needs some fixing. The solution I have implemented here is not as sophisticated as was discussed, but will work without conflicts in the specified screen dimensions.
Once that was done, this was implemented in all further table elements, since they seemed to be malfunctioning too. I have attached relevant GIF:

scrolling_finalfinal

README.md Outdated
@@ -536,6 +542,7 @@ Want to join this illustrious group? Have a look at the **[contributing guidelin
<!-- ALL-CONTRIBUTORS-LIST:START - Do not remove or modify this section -->
<!-- prettier-ignore-start -->
<!-- markdownlint-disable -->
<div class = "table-container">
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is a good idea. The bot might overwrite his 🤔

We should try using the config as suggested in the linked issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As for the others, this won't be a problem right? I'll remove the class for contributors table. Moreover, Jekyll is very limiting and the allcontributors configuration gives very few options. Even if we changed the contributorsPerLine to 3 (it is 4 now), the contributors tab will not change dynamically to screen size, but will show 3 per row for all screen sizes, which only looks awkward. I'll revert the ones in the contributors table.

@kaushikpadmanaban
Copy link
Contributor Author

kaushikpadmanaban commented Apr 22, 2021

Please review the final commit when you're available. I am unable to continue since Github on my side is unresponsive. I think I broke it. If it is approved, I will update my fork and try some other options. If you have any other suggestions, please let me know.

Edit: I understand this is not a fix, but a step. I was also wondering if I can add overflow attribute to the table element in CSS. I will look into it and let you know.

@sglavoie
Copy link
Member

@kaushikpadmanaban Chiming in to point out that you could wrap the table with a div (so you can give it an id as you did) as long as it's outside the tags generated by all-contributors. See: https://github.com/all-contributors/all-contributors-cli/blob/e9c1f55beb2c18391a5d5f0c9e8243dc3f89ebe3/src/generate/index.js#L5

I don't know what else you wanted to do so hopefully that bit of info is useful to you. 😄

@kaushikpadmanaban
Copy link
Contributor Author

kaushikpadmanaban commented Apr 22, 2021

Thanks for the suggestion @sglavoie! I was thinking about that exactly. I was unable to do that earlier since Github kept crashing. If you don't mind, I will close this pull request and create a new one once I make all the changes. All the commits I make in my fork reflect here and it is getting spammy.

@sglavoie
Copy link
Member

@kaushikpadmanaban I don't mind at all whatever way you go! I also see value in keeping everything related to this PR in the same place instead of starting from scratch (similar workflow explained in this article and a visual representation of squashing commits). My reasoning would be as follows:

  • As long as you don't force-push on top of your existing commits, what you are doing is clear and feedback can be added incrementally along the way. If the commits have not yet been reviewed, it's not really a bad thing to force-push, rebase and clean up in general depending on the context, like fixing a typo and doing git commit --amend – that's assuming you're doing that in a separate branch no one else is using of course 😄;
  • It's easy to just "squash" all your commit into one once all the necessary changes are made before merging into the master/main branch.

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

3 participants