Reduce gap on profile page #1512
Conversation
There was a problem hiding this comment.
Firstly, welcome! Thanks for your first contribution.
Has this been tested? I'm not sure this will work as it currently stands - the element that you've added that tries to reduce the margin is before the element that the margin is on.
Regardless, attempting to fix this by adding more markup/CSS isn't ideal - this can and should be fixed using the existing styles available. I'd suggest looking at the design framework to see what's available. In this case, adding a h-m-b-0 to the h1 and changing h-p-t-2 to h-p-t-0 on the profile container should resolve this without any additions.
|
Thank you for your feedback, I'll take a look at it again asap. |
|
Do note that the extra gap (likely an artifact of how things had looked before #1440 was fixed) is not caused by the header itself, but by the content that follows it (see other tabs for reference):
|
|
While we are at it, can you please take a look at filtering out the empty paragraph from from the profile description if the user doesn't have neither website, Twitter, or Discord link added (not an issue with your PR, but we might as well address it here)? This results in a an empty qpixel/app/views/users/show.html.erb Lines 27 to 48 in 91c0902 |
|
More on point, even with the changes the misalignment is still present, some further tweaking is needed - it's not as simple of a fix as just reducing the top padding (we should probably remove the padding on the left of the activity column as well while we are tweaking the layout as it's causes an additional visual shift). For reference, compare the updated version form the new PR (might look better with profile links filled in, but many users do not have those):
To how the header looks like on the "Vote Summary" tab:
|
|
Yes while editing I thought of removing the side padding but then thought it might be unnecessary! Sure I will implement the changes. Please let me know if I need to look into any other parts. Thank you so much. |
|
@2S00B5 doesn't seem like there's anything else for now, thanks! Left padding is optional, probably will need more thought put to it (don't recall the reason why it's there) |
app/views/users/show.html.erb
Outdated
| <<<<<<< Updated upstream | ||
| <div class="h-p-6 h-p-t-2"> | ||
| ======= | ||
| <div class="h-p-0 h-p-t-0"> | ||
| >>>>>>> Stashed changes |
There was a problem hiding this comment.
Note that you've committed with merge conflict marker active (note the system tests failing due to syntax errors). Suggested fix is to reset the branch to our upstream develop branch, manually reapply the changes, and force-push - looks like a stash apply has gone wrong.
There was a problem hiding this comment.
I apologize, this is my first time contributing to an open source project, so I followed the instructions for merging available online. Must not have worked out fine.
There was a problem hiding this comment.
Sorry for the hassles. Git can be a little finicky with merges sometimes. I needed some help from more experienced devs when I came here too. Please don't be discouraged; unfortunately it's kind of normal.
app/assets/stylesheets/users.scss
Outdated
| .reducegap { | ||
| font-weight: bold; | ||
| margin-bottom:-20px; | ||
| } | ||
|
|
There was a problem hiding this comment.
That (as well as its usage) seems like an artifact of updating the branch - hasn't @ArtOfCode- already review this and asked not to rely on overrides but use the design system instead?
There was a problem hiding this comment.
Yes I had removed that in the second branch, as I said I was just learning to force push in previous branch, that must have been missed out.
|
I have implemented the changes in the individual files manually for the gap removal. |
There was a problem hiding this comment.
Not sure how this has ended up here - there should be no dist folder here at all. The following git commands should amend your commits to remove it:
- Checkout a new temporary branch from this one. Make sure you're on this branch, then
git checkout -b temp-branch - Switch back to this branch:
git checkout 2S00B5/1480/large-textheader-gap - Reset back to the commit that added this file:
git reset --hard b65c61d - Remove the folder:
rm -r app/assets/stylesheets/dist - Add the change:
git add app/assets/stylesheets/dist - Amend the commit:
git commit -m "Implemented review" --amend - Cherry-pick the subsequent commits back to this branch:
git cherry-pick 28562e2^..4be3438
You can then delete the temporary branch and force-push to this one, which should solve this.
I have tried to reduce the gap between the user name and the succeeding text. Hope it's what you're looking for. And I have tested the changes in the inspect element on the webpage.
Fixes #1480.