-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add a link to the FSH Online Examples repo #142
Conversation
2367a85
to
be8503a
Compare
There was a problem hiding this 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', |
There was a problem hiding this comment.
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>! |
There was a problem hiding this comment.
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.
c228263
to
c82aa94
Compare
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:
or from @cmoesel:
|
Oh, you gotta go for the double pun. Twice the fun!
You think the joke's over and then BAM! -- there's another one! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it.
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: