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

Jump to bottom

Add a link to the FSH Online Examples repo #142

Merged
merged 4 commits into from
Sep 20, 2023
Merged

Add a link to the FSH Online Examples repo #142

merged 4 commits into from
Sep 20, 2023

Conversation

jafeltra
Copy link
Collaborator

@jafeltra jafeltra commented Sep 14, 2023 •
edited
Loading

Add a short blurb with a link to the repo that holds the examples. Previously, this was only in the README file, but it would be useful to have it in the app itself.

Two small pieces to consider when reviewing this:

  • Is the text too prominent? I could make it smaller and/or lighter, but I didn't want to get unnecessarily fancy. Let me know if you'd like screenshots of these options to consider. Edit: I decided to make the font size "small" so now this question is better posed as - is it too small? not prominent enough?
  • How do you feel about the wording? It's a little joke-y, so I could definitely simplify it with something just like "View and add to the examples collection on Github".

cmoesel
cmoesel requested changes Sep 19, 2023
View reviewed changes
Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

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

Thanks for this. I offered my opinion on the font size and made one suggestion regarding the link.

BTW - I didn't find the text too joke-y. After you warned us, I was expecting something along the lines of: Not enough FSH in the sea? Contribute...

src/components/FSHControls.js Outdated
},
dialogActionsMessage: {
fontStyle: 'italic',
fontSize: 'small',
Copy link
Member

Choose a reason for hiding this comment

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

I tried it both ways. Personally, I thought small was not quite prominent enough (after all, we've all been conditioned to ignore the fine print by now). So my vote is to revert back to your original styling.

src/components/FSHControls.js Outdated
<DialogActions className={classes.dialogActions}>
<div className={classes.dialogActionsMessage}>
Feel like an example is missing? Add to our collection of examples on{' '}
<Link href="https://github.com/FSHSchool/FSHOnline-Examples">GitHub</Link>!
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to link directly to the README part of the page (same page, but #readme anchor): https://github.com/FSHSchool/FSHOnline-Examples#readme

That way users see instructions on how to contribute right away.

@jafeltra
Copy link
Collaborator Author

Okay, I think @cmoesel is right and I didn't use a pun but I should have. Here are some options. Let me know if anyone likes one or a different combo of various fish-related puns:

Sea an example that is missing? Lend a kelp-ing hand and add it on GitHub!

Have an example that might be bene-fish-al? Seas the day and add to our collection on GitHub!

or from @cmoesel:

Not enough fish in the sea? Contribute an example on GitHub!

@cmoesel
Copy link
Member

cmoesel commented Sep 19, 2023

Oh, you gotta go for the double pun. Twice the fun!

Have an example that might be bene-fish-al? Seas the day and add to our collection on GitHub!

You think the joke's over and then BAM! -- there's another one!

cmoesel
cmoesel approved these changes Sep 19, 2023
View reviewed changes
Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

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

Love it.

@jafeltra jafeltra merged commit 97f6055 into master Sep 20, 2023
4 checks passed
@jafeltra jafeltra deleted the add-link-to-examples branch September 20, 2023 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmoesel cmoesel cmoesel approved these changes

@mint-thompson mint-thompson mint-thompson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jafeltra @cmoesel @mint-thompson