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

Add a page to get the suggestions for what to throw into the crib #89

Draft
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

joshprzybyszewski
Copy link
Owner

What broke / What you're adding

It'd be neat if we had a page that would show the calculated hand/crib pts for throwing different cards.

How you did it

Add a new directory for Suggestions. Currently, it will only suggest what to throw into the crib, but it'd also be neat to suggest what to peg next.

How to test it and how to try to break it

Comment on lines 65 to 73
const updateValue=(_) => {
// TODO figure out how to get scroll capturing to work
// TODO update the value of this card in the state
console.log(`scrolled: ${card}`);
};
const updateSuit=(_) => {
// TODO update the state so that this card increments suits
console.log(`clicked: ${card}`);
};
Copy link
Owner Author

Choose a reason for hiding this comment

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

These are the next things I'd like to do

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like scrolling --> change number, clicking --> change suit? Nice.

Comment on lines 30 to 36
<TableRow>
<TableCell>Hand Points (avg)</TableCell>
<TableCell>Hand Points (median)</TableCell>
<TableCell>Crib Points (avg)</TableCell>
<TableCell>Crib Points (median)</TableCell>
<TableCell>Throw</TableCell>
</TableRow>
Copy link
Owner Author

Choose a reason for hiding this comment

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

It would be awesome if we could get a table that has "merged cells" for the first header row, then has sub columns in the second header row so that we put | hand points | crib points | throw | in the first row and have avg | median | max as "sub columns" under each hand and crib. I haven't been able to figure that out yet, but it seems like this page should be able to help?

@codecov
Copy link

codecov bot commented Mar 6, 2021

Codecov Report

Merging #89 (c9be6aa) into master (3f31648) will increase coverage by 0.06%.
The diff coverage is n/a.

❗ Current head c9be6aa differs from pull request most recent head 5c41068. Consider uploading reports for the commit 5c41068 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
+ Coverage   70.07%   70.13%   +0.06%     
==========================================
  Files          80       80              
  Lines        3465     3465              
==========================================
+ Hits         2428     2430       +2     
+ Misses        819      818       -1     
+ Partials      218      217       -1     
Impacted Files Coverage Δ
server/play/cut.go 50.00% <0.00%> (+5.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f31648...5c41068. Read the comment docs.

Comment on lines 20 to 32
<Grid
item
container
spacing={1}
><GridList>
{handCards.map((card, index) => (
<ChoosingCard
key={`handcard${index}`}
card={card}
/>
))}
</GridList>
</Grid>
Copy link
Owner Author

Choose a reason for hiding this comment

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

🔍 This could probably be split out into its own component?

Comment on lines 5 to 7
handCards: [
'AH', 'KH', '5H', 'JH', '8C', '3D',
],
Copy link
Owner Author

Choose a reason for hiding this comment

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

🔍 I need a way to update this array

Copy link
Collaborator

Choose a reason for hiding this comment

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

Write a reducer below that takes a string and an index to update?

Comment on lines 25 to 29
setSuggestionResult: {
reducer: (state, action) => {
state.suggestedHands = action.payload.suggestions;
},
},
Copy link
Owner Author

Choose a reason for hiding this comment

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

🔍 This is currently unused.

Copy link
Owner Author

@joshprzybyszewski joshprzybyszewski left a comment

Choose a reason for hiding this comment

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

I'd like to have the client-side set up to just be using dummy data. Once I can edit the input hand adequately and see things update acceptably, I'd like to implement a REST endpoint on the server to accept the input and spit out the suggestions. Someday, it'd be neat if the client would just do all of the calculations

Comment on lines 77 to 79
onScroll={updateValue}
onClick={updateSuit}
className={classes.root}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is our formatting messed up? I guess I need to revisit our tooling to see if we're formatting on commit

@cszczepaniak
Copy link
Collaborator

Reading through this code again makes me sorely miss TypeScript. I think we're going to want that ASAP

@@ -6,6 +6,7 @@ import ListItem from '@material-ui/core/ListItem';
import ListItemIcon from '@material-ui/core/ListItemIcon';
import ListItemText from '@material-ui/core/ListItemText';
import AddCircleOutlineIcon from '@material-ui/icons/AddCircleOutline';
import SearchIcon from '@material-ui/icons/Search';
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's kinda cool and all that our linter comments on PRs but...

Can we just

  1. Run lint on commit (with husky and lint-staged)
  2. Run lint on PR builds

Number 1 should realistically prevent 99% of these lint comments from showing up.

Comment on lines 3 to 6
import Card from '@material-ui/core/Card';
import CardContent from '@material-ui/core/CardContent';
import { makeStyles } from '@material-ui/core/styles';
import Typography from '@material-ui/core/Typography';
Copy link
Collaborator

@cszczepaniak cszczepaniak Mar 6, 2021

Choose a reason for hiding this comment

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

🌵 These should prolly just use the named imports from '@material-ui/core'

height: 160,
},
value: {
fontSize: 14,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should keep font sizes in rem so they stay relative to a single reference size. Root font size is probably 16, so this would become '0.875rem'

Comment on lines 66 to 67
// TODO figure out how to get scroll capturing to work
// TODO update the value of this card in the state
Copy link
Collaborator

Choose a reason for hiding this comment

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

For managing state, you could do:

// near the top of your component
const [index, setIndex] = useState(0);

// ...
const updateValue = () => setIndex(prev => prev + 1); // or - 1, depending on scroll direction

I'm not sure what data onScroll feeds back to you, but that's at least the state management part

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh wait, you want to update it in redux probably...

const SuggestionsTable = () => {
const sugs = useSelector(selectSuggestions);

if (!Array.isArray(sugs)) {
Copy link
Collaborator

@cszczepaniak cszczepaniak Mar 6, 2021

Choose a reason for hiding this comment

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

cries in typescript

spacing={1}
>
<GridList>
{sug.throw.map((card, index) => (
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should probably not name things throw since that's a keyword in JS

@cszczepaniak
Copy link
Collaborator

Glad to see cribbage is alive again 😎

…uggest

 Conflicts:
	logic/strategy/calculated_crib.go
	logic/strategy/calculated_hand.go
	logic/suggestions/utils.go
	network/suggest.go
	server/server.go
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.

2 participants