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

Footer Section Revamp #2439

Open
wants to merge 9 commits into
base: homepage-revamp
Choose a base branch
from
Open

Conversation

akshaaatt
Copy link
Member

@akshaaatt akshaaatt commented Mar 3, 2022

The Footer is a consistent layout present in all our pages hence it should make sure to highlight our fellow projects and important links.
Screenshot from 2022-04-03 18-15-15

Screenshot from 2022-04-03 18-15-35

@akshaaatt akshaaatt self-assigned this Mar 3, 2022
@reosarevok reosarevok changed the base branch from homepage-revamp to master March 3, 2022 12:08
@reosarevok reosarevok changed the base branch from master to homepage-revamp March 3, 2022 12:08
@akshaaatt akshaaatt changed the base branch from homepage-revamp to master March 7, 2022 14:36
@akshaaatt akshaaatt changed the base branch from master to homepage-revamp March 7, 2022 14:36
@akshaaatt akshaaatt force-pushed the akshat/footer-revamp branch 6 times, most recently from 4324258 to 1b61d89 Compare March 10, 2022 07:47
@akshaaatt akshaaatt force-pushed the akshat/footer-revamp branch 3 times, most recently from 32c8e4b to be575b5 Compare April 1, 2022 21:37
@akshaaatt
Copy link
Member Author

@brainzbot retest please

@akshaaatt
Copy link
Member Author

@brainzbot retest please

@akshaaatt akshaaatt force-pushed the akshat/footer-revamp branch 3 times, most recently from e2de953 to 0532889 Compare April 2, 2022 21:52
Copy link
Member

@mwiencek mwiencek left a comment

Choose a reason for hiding this comment

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

Just to give some general feedback unrelated to my comments below: I think the font size of the footer is a few notches too big. :) Especially for something that will appear on every page. But let's hear what @reosarevok and @yvanzo think!

root/layout/components/Head.js Outdated Show resolved Hide resolved
root/layout/components/Footer.js Outdated Show resolved Hide resolved
root/layout/components/Footer.js Outdated Show resolved Hide resolved
Comment on lines 133 to 155
<span className="fs-4">
{l('Development IRC: ')}
</span>
<a
className="fw-bold fs-4"
href="https://kiwiirc.com/nextclient/irc.libera.chat/?#metabrainz"
rel="noopener noreferrer"
target="_blank"
>
{l('#metabrainz')}
</a>
Copy link
Member

Choose a reason for hiding this comment

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

#musicbrainz is the more general channel for the project, though I think it makes sense to just link to our IRC documentation, as the existing footer does, since that lists all of our channels and has other useful information. That would also allow people use their own IRC client.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, added both the IRC channels and tweaked the UI a bit!

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, but that wasn't my suggestion. :) By "link to our IRC documentation, as the existing footer does," I meant https://musicbrainz.org/doc/Communication/IRC. This contains useful information and lets people connect with their own client. If I was looking to join a project's IRC channel, I wouldn't want a direct link to some web client: I'd want to read about what the purpose of each channel is first, and then connect with my own client.

Comment on lines 149 to 175
<a
className="fw-bold fs-4"
href="mailto:support@metabrainz.org"
>
{l('support@metabrainz.org')}
</a>
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, we probably just want a contact link to https://metabrainz.org/contact so that people read that before contacting us? @reosarevok would have more relevant input than me since he deals with support.

<div className="row mt-4">
<div className="col-md-3 border-top pt-4 d-none d-md-block fs-4">
<p>
{l('OSS Geek? ')}
Copy link
Member

Choose a reason for hiding this comment

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

I noticed there are several strings with errant trailing whitespace (please search for them in your editor).

Suggested change
{l('OSS Geek? ')}
{l('OSS Geek?')}

Copy link
Member Author

Choose a reason for hiding this comment

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

Right!

true,
)}
</button>
<ul aria-labelledby="languageDropdown" className="dropdown-menu">
Copy link
Member

Choose a reason for hiding this comment

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

We use kebab-case for class names and identifiers.

Suggested change
<ul aria-labelledby="languageDropdown" className="dropdown-menu">
<ul aria-labelledby="language-dropdown" className="dropdown-menu">

Copy link
Member Author

Choose a reason for hiding this comment

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

RIght!

Comment on lines 544 to 555
{l('Brought to you by')}
<div className="image ms-1 me-1">
<img
alt="MetaBrainz"
height="24"
src="../../static/images/meb-icons/MetaBrainz.svg"
width="24"
/>
</div>
<span>
{l('MetaBrainz Foundation')}
</span>
Copy link
Member

Choose a reason for hiding this comment

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

"Brought to you by MetaBrainz Foundation" should be one string; can we maybe inject a <span /> into the string and display the image with CSS instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 308 to 310
<div className="bs">
<Footer />
</div>
Copy link
Member

Choose a reason for hiding this comment

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

It seems like we can add the bs class inside Footer and not have to wrap it in a div.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

</div>
<div className="col-sm-12 col-md-2">
<h3 className="fs-2 fw-bold color-black">
{l('Join Us')}
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this shouldn't come before "Fellow Projects," because this is more relevant to MusicBrainz.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have moved the Join Us column before fellow projects and shifted the Useful Links column to the last which solves this problem!

@akshaaatt akshaaatt force-pushed the akshat/footer-revamp branch 3 times, most recently from 005e452 to e78162e Compare April 3, 2022 13:22
@akshaaatt
Copy link
Member Author

@brainzbot retest please

mwiencek added a commit to metabrainz/design-system that referenced this pull request Jun 2, 2022
metabrainz/musicbrainz-server#2439 is copying a
bunch of logo assets into musicbrainz-server, but I think they should
just be published with design-system.  I imagine other projects will
find them useful too.

I've left brand/icons/ npmignored for now until we have a use for it,
and brand/logos/development/ too, which is only useful for development
and historical purposes.
@santiagofn
Copy link
Contributor

@brainzbot retest please

@yvanzo
Copy link
Contributor

yvanzo commented Apr 27, 2023

@brainzbot please retest this.

@reosarevok
Copy link
Member

@brainzbot, retest this please

@akshaaatt akshaaatt changed the base branch from homepage-revamp to akshat/header-revamp October 4, 2023 15:49
@akshaaatt akshaaatt changed the base branch from akshat/header-revamp to homepage-revamp October 4, 2023 15:49
@metabrainz metabrainz deleted a comment from akshaaatt Oct 6, 2023
@mwiencek mwiencek force-pushed the akshat/footer-revamp branch 2 times, most recently from ebf8a37 to cf62bc1 Compare October 6, 2023 12:38
akshaaatt and others added 8 commits October 6, 2023 18:37
- Created a file with the raw input data used to build the footer
- Most parts of the footer were moved to their own components
- Row-cols is used to define the layout on different breakpoints
- Added margins on breakpoints < xl
- Fixed margins in first block
- Added text about MusicBrainz
- Change channel's list line-height
- Added gray style
- Removed <bold> from links
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants