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.

Sign up for GitHub

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

fix: Implemented the logic to force the user to tag at least one repositor… #2007

Merged
merged 5 commits into from
Oct 26, 2023
Merged

fix: Implemented the logic to force the user to tag at least one repositor… #2007

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b26bae4
Implemented the logic to force the user to tag at least one repositor…
shamimbinnur Oct 26, 2023
cd9f150
Removed console log
shamimbinnur Oct 26, 2023
bc10ca8
Changed wording
shamimbinnur Oct 26, 2023
ed98a83
Wording changes
shamimbinnur Oct 26, 2023
ddcdc12
Wording changes
shamimbinnur Oct 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions components/molecules/HighlightInput/highlight-input-form.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,12 @@ const HighlightInputForm = ({ refreshCallback }: HighlightInputFormProps): JSX.E

const taggedRepoFullNames = taggedRepoList.map((repo) => `${repo.repoOwner}/${repo.repoName}`);

// Check if the user has tagged at least one repo and ask them to tag at least one if they haven't
if (taggedRepoList.length < 1) {
setError("Please tag at least one repo!");
Copy link
Member

@nickytonline nickytonline Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

wording (blocking): Make it clear why at least one repository needs to be added.

Suggested change
setError("Please tag at least one repo!");
setError("Please add at least one repository associated with your blog post");

@BekahHW, any suggestions for better wording?

Copy link
Member

@BekahHW BekahHW Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, I think this is tricky. I need some clarification first. The problem is that when someone submits a blog post, they are automatically generating a repo, right?

I think the repo should be optional in for blog post highlights. For example, it's still valid to highlight work on something like "A guide to writing documentation." There's not necessarily a repo associated with that. For the blog post mentioned in the issue, I'm not sure what repo I would tag.

Can we conditionally check if the highlight is a blog post, and then leave the input empty unless they put something in there?

Copy link
Member

Choose a reason for hiding this comment

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

@bdougie thoughts on the above?

Copy link
Member

Choose a reason for hiding this comment

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

If we move forward with this implementation, I think Nick's copy works just change the to your:

"Please add at least one repository associated with your blog post"

Copy link
Member

Choose a reason for hiding this comment

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

Good point that it needs to be when a blog post is added @BekahHW. Missed that. 😅

@shamimbinnur, we'd need to check that it's a blog post before showing that error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @nickytonline, thanks for suggesting the better wording.

Not allowing the user to post a blog without tagging the repo is intentional. As @bdougie's comment here states, "every blog highlight should be linked to a repo."

If no repo is tagged whether automatically or manually an empty array of repos is passed to the backend. However, the backend does not keep it empty; it just splits text from the blog link and creates an invalid repo URL.

Let me be clear, "Does every blog post highlight need to be linked to a repo?"

CC: @BekahHW @bdougie

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so you're saying from a backend perspective, we have to have a repo listed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically yes!

return;
}

if (res.isError) {
setLoading(false);

Expand Down
Loading