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

BB-721: Add placeholder text for empty sections #983

Merged
merged 14 commits into from
May 22, 2023

Conversation

Bigguysahaj
Copy link
Contributor

@Bigguysahaj Bigguysahaj commented Apr 3, 2023

Problem

Solved the problem of placeholder text for empty sections and call to action for creating new collections, identifiers, and relationships.

As mentioned in the ticket
https://tickets.metabrainz.org/browse/BB-721?jql=labels%20%3D%20good-first-bug%20ORDER%20BY%20created%20DESC
and the pull request #947

Solution

Before:
image

After:
image

I added placeholder text, along with a link that directs users directly to the /edit endpoint.

Areas of Impact

Main areas of impact were the EntityRelatedCollections , EntityRelationships & EntityIdentifiers functions.

@kellnerd
Copy link
Contributor

kellnerd commented Apr 3, 2023

@Bigguysahaj When you change this PR's title to the correct format "BB-721: Add placeholder text for empty sections" it will automatically be linked to the JIRA ticket.

@Bigguysahaj
Copy link
Contributor Author

@Bigguysahaj When you change this PR's title to the correct format "BB 721: Add placeholder text for empty sections" it will automatically be linked to the JIRA ticket.

Ohh I'm sorry, thanks tons I will fix it.

@Bigguysahaj Bigguysahaj changed the title BB-721 - Add placeholder text for empty sections BB 721: Add placeholder text for empty sections Apr 3, 2023
@kellnerd
Copy link
Contributor

kellnerd commented Apr 4, 2023

No problem, we all make mistakes... just as I did when I forgot the hyphen between BB and the ticket number in my above suggestion. Please change it again to "BB-721: Add placeholder text for empty sections" and it should work for real 😅

@Bigguysahaj Bigguysahaj changed the title BB 721: Add placeholder text for empty sections BB-721: Add placeholder text for empty sections Apr 4, 2023
@Bigguysahaj
Copy link
Contributor Author

No problem, we all make mistakes... just as I did when I forgot the hyphen between BB and the ticket number in my above suggestion. Please change it again to "BB-721: Add placeholder text for empty sections" and it should work for real 😅

@kellnerd could you please help me understand how to fix these ESLint errors, and further more how to go about to get my PR merged?

@kellnerd
Copy link
Contributor

kellnerd commented Apr 11, 2023

Well, the ESLint errors tell you where you haven't formatted the code correctly. So you have to format it to fulfil ESLint's expectations in order to make the errors disappear.
Just google the error messages which you don't understand or look them up in the ESLint docs.
You can also run ESLint locally to check whether you've fixed all errors, simply run npm's lint script command to do that, e.g. by executing npm run lint.

After fixing the errors you have to be patient and wait until someone with merge priviledges (i.e. not me) reviews your PR.

@allgandalf
Copy link
Contributor

allgandalf commented Apr 13, 2023

No problem, we all make mistakes... just as I did when I forgot the hyphen between BB and the ticket number in my above suggestion. Please change it again to "BB-721: Add placeholder text for empty sections" and it should work for real sweat_smile

@kellnerd could you please help me understand how to fix these ESLint errors, and further more how to go about to get my PR merged?

Hey @Bigguysahaj, there is Indentation problem and some syntax errors in your submitted PR, fix the indentation while your PR gets reviewed.

@Bigguysahaj
Copy link
Contributor Author

Thanks alot, I will fix that. @RohanSasne

@MonkeyDo
Copy link
Member

Hello @Bigguysahaj !

Would you like to continue on this PR?
Let me know if you need any help or advice :)

@Bigguysahaj
Copy link
Contributor Author

I think I fixed the Eslint and some basic errors, thanks to Kellnerd and Rohan.
Is this PR okay? and should I squash all my commits into one?

@allgandalf
Copy link
Contributor

well i guess, it would be okay, the changes are clear and readable so i don’t think that’s an issue, but let monkey have a say in this 😁

@Bigguysahaj
Copy link
Contributor Author

Okay, @MonkeyDo, could you please review the changes and let me know if I need to do something else?

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Thank you for coming back to it !

I have some wording suggestions as well as some small nitpicking, but overall this does a great job already, thank you :)

src/client/components/pages/entities/identifiers.js Outdated Show resolved Hide resolved
src/client/components/pages/entities/relationships.js Outdated Show resolved Hide resolved
src/client/components/pages/entities/identifiers.js Outdated Show resolved Hide resolved
src/client/components/pages/entities/relationships.js Outdated Show resolved Hide resolved
Bigguysahaj and others added 7 commits May 18, 2023 19:36
Co-authored-by: Monkey Do <MonkeyDo@users.noreply.github.com>
Co-authored-by: Monkey Do <MonkeyDo@users.noreply.github.com>
Co-authored-by: Monkey Do <MonkeyDo@users.noreply.github.com>
Co-authored-by: Monkey Do <MonkeyDo@users.noreply.github.com>
Co-authored-by: Monkey Do <MonkeyDo@users.noreply.github.com>
Co-authored-by: Monkey Do <MonkeyDo@users.noreply.github.com>
Co-authored-by: Monkey Do <MonkeyDo@users.noreply.github.com>
@Bigguysahaj
Copy link
Contributor Author

Bigguysahaj commented May 18, 2023

Thank you for coming back to it !

I have some wording suggestions as well as some small nitpicking, but overall this does a great job already, thank you :)

Thank you for reviewing, it was really helpful : )
I had no idea you could use this

{relationships?.length > 0 ? (

instead of

{relationships && relationships.length > 0 ? (

@Bigguysahaj Bigguysahaj requested a review from MonkeyDo May 22, 2023 16:20
Fix ESLint warning
Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Good to go, thank you very much !

@MonkeyDo MonkeyDo merged commit e6c376e into metabrainz:master May 22, 2023
@Bigguysahaj Bigguysahaj deleted the add-placeholder-text branch May 23, 2023 10:46
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.

4 participants