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

Intro Section in Homepage #2471

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

Intro Section in Homepage #2471

wants to merge 20 commits into from

Conversation

akshaaatt
Copy link
Member

This section highlights the first few features that a user encounters on the website.

akshaaatt and others added 10 commits March 7, 2022 18:41
This also sets up a minimal bootstrap setup we can expand on,
and fontawesome icons.
These were converted to Flow syntax by hand from the official TypeScript
types.
Now that we have the fontawesome types, we can set this to strict.
Correct index.js

Fix About.js

Fix css

Fix css

Finalize About css

Update About.js
Facts section added to the homepage

Add facts

Add facts css

Correct index.js

Correct index.js

Fixed Facts.js

Updated facts

Use spacing class for Facts.js

Finalize Facts css

Fix Facts section
@akshaaatt akshaaatt changed the base branch from master to homepage-revamp March 31, 2022 10:52
akshaaatt and others added 8 commits April 4, 2022 21:28
Fix Explore.js

Fix homepage.less

Simplify css and correct the descriptions to have uniformity

Fix css for title

Fix css for title

Simplify css

Simplify code and make changes based on reviews

Fix changes according to reviews

Fix changes

Remove fixed height of card

Update homepage.less

Small fix

Update Explore.js

Update Explore.js

Update Explore.js

Update Explore.js
Add footer.less and fix css
Update Footer.js

Fix tests and cleanup

Update MBS-4555.json5

Update MBS-4555.json5
Fix tests

Cleanup

Update Footer.js

Update MBS-9669.json5

Fix bootstrap js inclusion

Hydrate Footer component

Update Footer.js

Fix review comments
@akshaaatt akshaaatt force-pushed the akshat/intro-new branch 5 times, most recently from 3e389fd to 271b30e Compare April 25, 2022 17:13
Copy link
Member

@reosarevok reosarevok left a comment

Choose a reason for hiding this comment

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

Some comments for now based on what is on test.mb :)

Comment on lines 66 to 69
<FontAwesomeIcon
icon={faBlog}
size="lg"
/>
Copy link
Member

Choose a reason for hiding this comment

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

I understand maybe having no "Twitter" text for Twitter, but I really don't think this icon is self-explanatory as "Blog". Can we have "Blog" as text by it?

{key: 6, label: 'Area'},
{key: 7, label: 'Place'},
{key: 8, label: 'Annotation'},
{key: 9, label: 'CD Stud'},
Copy link
Member

Choose a reason for hiding this comment

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

Stub, not Stud :)

Comment on lines +23 to +39
const chipData = [
{key: 0, label: 'Artist'},
{key: 1, label: 'Release'},
{key: 2, label: 'Recording'},
{key: 3, label: 'Label'},
{key: 4, label: 'Work'},
{key: 5, label: 'Release Group'},
{key: 6, label: 'Area'},
{key: 7, label: 'Place'},
{key: 8, label: 'Annotation'},
{key: 9, label: 'CD Stud'},
{key: 10, label: 'Editor'},
{key: 11, label: 'Tag'},
{key: 12, label: 'Instrument'},
{key: 13, label: 'Series'},
{key: 14, label: 'Event'},
{key: 15, label: 'Documentation'},
];
Copy link
Member

Choose a reason for hiding this comment

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

Showing all of the search options by default is a bit intimidating. A bunch of these (like CD stubs, tags, annotations) are not even entities, even though the search field says "search X entities" :) I'd suggest displaying the few most relevant options by default (Artist, Label, Release maybe?) with a button to show all options.

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 am fine with a dropdown to showcase the other entities as you suggest but yeah showing them all nicely can be a good flex if we wanna, haha! :)


export default class Intro extends React.Component {
state = {
data: 'Actively looking for a barcode...',
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused about this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing the incomplete barcode from this PR. Can make that addition later.

className="ms-4 mt-2"
style={{color: 'white', height: '5vh'}}
>
{l(`World's Biggest Open Source Music Database`)}
Copy link
Member

Choose a reason for hiding this comment

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

Only claim this if you're certain. I'm not, but I also haven't done the research.

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'm not sure whether there is a downside to claiming. Almost every company I see claims that they are the biggest. Tbh I do believe (without exhaustive research) that MusicBrainz is the largest open-source music database. This statement in any case is powerful and is definitely a pick for me. (aerozol suggested this text btw! :) )

return false;
}}
placeholder="Search 41,054,421 Entities"
style={{textTransform: 'capitalize'}}
Copy link
Member

Choose a reason for hiding this comment

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

Please don't. Not only is it confusing to see the text displayed differently from what I typed, but it's not even correct in many languages (such as Spanish or French) to capitalize every word of a title.

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. Definitely needs to be reverted.

Comment on lines 111 to 121
<div className="col-4 col-md-2">
<button
className="btn btn-primary"
onClick={searchButtonClick}
type="button"
>
<FontAwesomeIcon
icon={faSearch}
size="lg"
/>
</button>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can actually have this touch the end of the search bar rather than float?

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 122 to 131
<button
className="btn btn-b-n"
onClick={this.handleShow}
type="button"
>
<FontAwesomeIcon
icon={faScanner}
size="lg"
/>
</button>
Copy link
Member

Choose a reason for hiding this comment

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

This button does not show on test at the moment so I'm not sure what the point is supposed to be (https://fontawesome.com/icons/scanner?s=solid ?) but I'm confused about needing a second button other than "search".

Edit: Oh, I understand reading a few lines later that the idea is to have a way to scan a barcode - is this something anyone has expressed an interest on? I'm a bit confused by the addition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Try out the button placed on https://musicbrainz-web.web.app @reosarevok

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed this for now.

Comment on lines 186 to 95
<div
className="row ps-2 pe-2"
style={{height: '40vh', overflow: 'hidden'}}
>
{this.props.recentAdditions.map((artwork, index) => (
<ReleaseArtwork
artwork={artwork}
key={index}
/>
))}
</div>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for the weirdly off-center, distracting background squares in test? Or is it a bug? It looks like a bug, but I'm not sure :)

@@ -11,6 +11,53 @@
}
}

.intro {
background: linear-gradient(to right, @musicbrainz-purple 0%, @musicbrainz-orange 100%);
Copy link
Member

Choose a reason for hiding this comment

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

All this color seems a bit overpowering to me, but dunno.

Update SearchIcon.js

Font Awesome types and minor UI fix

Fixed the search bar inside the header and combined the username menu item and the my data menu item.

Fix tests

Fix tests

Update font size
@akshaaatt akshaaatt marked this pull request as ready for review April 30, 2022 18:14
Fix package.json

Update Intro section

Fix tests

Fix typo

Fix implementations

Fix artwork implementation

Remove min-width for fixing responsive issue

Fix css of pages

Cleanup
@akshaaatt akshaaatt force-pushed the akshat/intro-new branch from 1ed6dfe to c5a3b76 Compare May 9, 2022 16:23
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