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

Initial commit for ravamping footer #545

Closed
wants to merge 1 commit into from

Conversation

frappelatte28
Copy link
Contributor

Link to discussion

Problem

UI of footer is less intuitive and less user engaging.

Solution

Revamped the footer as discussed in the link provided above.

Areas of Impact

src/client/components/footer.js
src/client/stylesheets/style.less

@coveralls
Copy link

Coverage Status

Coverage increased (+0.006%) to 60.853% when pulling ce2edaa on frappelatte28:dev into 229437a on bookbrainz:master.

@frappelatte28
Copy link
Contributor Author

@MonkeyDo Firstly, I apologize for being so late, I was busy with my semester exams.

I have made an initial commit for revamping the footer component, I have some following doubts regarding the styling process used in codebase.

  • As I can see we are using an older version of bootstrap and react-bootstrap I am a bit confused how to center items inside <Row> element. I do believe we should not be using any inline styling for this purpose and we can also avoid writing css in external files if we can leverage predefined classNames of Bootstraps. In bootstrap 5 we do this using justify-content-center.
    But I am not sure about bootstrap 3.
  • I am also not sure about how to provide font-sizes using bootstrap classNames that we follow here
  • What is the best way to assign colors to elements, like should we use color variables or hard-coded color names. I do think using color variables is a great option. But I am unsure where to place the color variables.

It would be nice if you can guide me on that

@MonkeyDo
Copy link
Contributor

@MonkeyDo Firstly, I apologize for being so late, I was busy with my semester exams.

I have made an initial commit for revamping the footer component, I have some following doubts regarding the styling process used in codebase.

  • As I can see we are using an older version of bootstrap and react-bootstrap I am a bit confused how to center items inside <Row> element. I do believe we should not be using any inline styling for this purpose and we can also avoid writing css in external files if we can leverage predefined classNames of Bootstraps. In bootstrap 5 we do this using justify-content-center.
    But I am not sure about bootstrap 3.
  • I am also not sure about how to provide font-sizes using bootstrap classNames that we follow here
  • What is the best way to assign colors to elements, like should we use color variables or hard-coded color names. I do think using color variables is a great option. But I am unsure where to place the color variables.

It would be nice if you can guide me on that

Thanks for opening this PR!
My turn to apologize about taking a long time to get to it :)

Regarding your questions:

  • The class text-center can be used to center elements horizontally in its parent. in fact it's in use in the current footer.
  • Regarding font sizes, I think the way to go would be to define that in the LESS/CSS styles, especially if there are other css rules you'll need to define for footer elements. Otherwise, there are a few classes that could maybe do the trick: .h1, .h2.h6.
    In any case we agree that we should avoid inline styles if possible :)
  • I'm a proponent of using variables whenever possible, and since we use LESS that's easy. You can define your variable like @mycolor: #fff; in the style.less file like we do here: https://github.com/bookbrainz/bookbrainz-site/blob/master/src/client/stylesheets/style.less#L37-L39
    That style.less file it getting quite long at this point, so if there are more colors or other variables you'll need to define, we could create a new variables.less file and move any LESS variable in it, then import that file in style.less right after @import "./lobes/less/theme.less";.

<Row >
<Col xs={6} xsOffset={3}>
<Row>
<Col xs={2} >
Copy link
Contributor

Choose a reason for hiding this comment

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

For centering these items properly, here's the structure I ended up with:
Capture d’écran 2021-01-13 à 18 19 44

The modifications mainly require using <Col xs={3} >, since we have 4 elements and the bootstrap grid is arranged in 12 columns. With 4*3 col wide we use the full space of the parent element, while the parent's xs={6} xsOffset={3} ensures it is all centered.

/>
</a>
</small>
<Grid fluid className="padding-top-1 padding-bottom-1">
Copy link
Contributor

Choose a reason for hiding this comment

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

I've had better results adding these padding-top-1 padding-bottom-1 to this Grid's two Row children

@frappelatte28
Copy link
Contributor Author

BB_footer
BB_footer1

Hey @MonkeyDo, As you can see in the new footer design the "See all revisions " button is not visible because the new footer has more height than the previous one. We have this button in our codebase in the "index.js" file but I am unable to find the file where I can give margin and make space in between them.
Also visited link is showing pink color which is not aesthetically pleasing, and do we really want to change the color of every visited link or our aim was to only change the colors of visited books.
And one more thing, as I mentioned that icons will change color or hovering, I did that but I don't know how to have that rounded border around icons, I tried to set border-radius 50% but as icons don't have equal width and height, it's not giving the result I expected. If possible can I use simple font-awesome icons instead of react-font awesome icons?

@MonkeyDo
Copy link
Contributor

As you can see in the new footer design the "See all revisions " button is not visible because the new footer has more height than the previous one. We have this button in our codebase in the "index.js" file but I am unable to find the file where I can give margin and make space in between them.

This is where we define the bottom margin size for two different screen sizes: https://github.com/bookbrainz/bookbrainz-site/blob/master/src/client/stylesheets/style.less#L67-L72

Also visited link is showing pink color which is not aesthetically pleasing, and do we really want to change the color of every visited link or our aim was to only change the colors of visited books.

You'll find that here: https://github.com/bookbrainz/bookbrainz-site/blob/master/src/client/stylesheets/style.less#L263-L265
We surely want to keep the standard browser mechanism of showing any visited link in a different color thatn a non-visited one, but we can adjust the different colors to work with the background color.

And one more thing, as I mentioned that icons will change color or hovering, I did that but I don't know how to have that rounded border around icons, I tried to set border-radius 50% but as icons don't have equal width and height, it's not giving the result I expected. If possible can I use simple font-awesome icons instead of react-font awesome icons?

That seems to me like the wrong way around to go :)
If the CSS isn't currently working properly, the best way to solve it will be to continue working on the CSS rather than change the contents.
There are multiple ways to go about it in this case; here's a proposition:

Considering we need a specific equal width and height for the 50% round border, let's not use the bootstrap col-xs-… which are percentage-based.
Instead, let's replace it with the existing round-color-icon class and modify it for use in the footer.
Below you'll find a bit of LESS code that solves the issue; you'll probably want to adjust it accordingly.

I also removed the padding-top-1 and padding-bottom-1 classes from the footer and moved them to each section of the footer instead and added a text-center (now that we won't use the col-xs- classes) and ended up with this for the round icons parent: <Col xs={6} xsOffset={3} className="text-center padding-bottom-d8 padding-top-d8">

// Here I'm only modifying the .round-color-icon class
// inside the footer, in case it is used somewhere else

footer .round-color-icon {
	// Adjust size here as desired.
	// For a round icon keep width and height equal
	width: 4.5em;
	height: 4.5em;
	margin-right: 2em;
	
	// Color with and without hovering
	background-color: inherit;
	&:hover{
		background-color: #754e37;
	}
	
	// Direct children of round-color-icon
	& > * {
		min-width: 100%; // Overwrite existing style
		justify-content: center; // Vertical alignment for flex items
		&:hover{
			color: #eb743b;	// Adjust color on hover
		}
	}
}

The result so far:
Capture d’écran 2021-01-15 à 14 23 57

@MonkeyDo
Copy link
Contributor

Closed in favor of #739, as we are harmonizing the footers between all MetaBrainz projects

@MonkeyDo MonkeyDo closed this Feb 17, 2022
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.

3 participants