-
Notifications
You must be signed in to change notification settings - Fork 29
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
Assign a single meta at puzzle creation time #736
Changes from 5 commits
534328e
e0b0395
f83997a
36d67be
3688e68
2c1d67b
0bf6063
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
{ | ||
"manifest_version": 3, | ||
"name": "Cardboard Chrome Extension", | ||
"version": "1.0", | ||
"version": "1.0.1", | ||
"icons": { | ||
"16": "images/cardboard_bird.png", | ||
"32": "images/cardboard_bird.png", | ||
|
@@ -16,8 +16,8 @@ | |
"cookies" | ||
], | ||
"host_permissions": [ | ||
"0.0.0.0:*", | ||
"http://localhost:*/*", | ||
"http://*:*/*", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels kind of sketchy, was this just to include 127.0.0.1 or was there more? I guess it's ok as a temporary hack if we need to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, would this potentially get flagged during the Chrome Extension store review? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rgossiaux, didn't you say you use 0.0.0.0? @akirabaruah I actually already included this in the most recent extension upload and it was accepted. However, maybe this is why it took longer. https://developer.chrome.com/docs/extensions/develop/concepts/match-patterns has Chrome's suggestions. We can include just
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah thanks for the clarifications. SGTM. I guess we want the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. I'm not authorizing using OAuth. I'm instead reading the csrftoken from the website's cookie. This is the list of URLs whose cookies I can read from. The asterisks are needed for folks various ports if you want to test it locally. The access to read any website's title and URL is from the permissions's activeTab There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah I do, I was wondering why you changed to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rgossiaux I thought |
||
"http://localhost/*", | ||
"https://cardboard.rocks/*", | ||
"https://www.cardboard.rocks/*" | ||
] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,17 @@ | ||
import React from "react"; | ||
import { useDispatch } from "react-redux"; | ||
import { useDispatch, useSelector } from "react-redux"; | ||
import Modal from "react-bootstrap/Modal"; | ||
import Button from "react-bootstrap/Button"; | ||
import Form from "react-bootstrap/Form"; | ||
import { addPuzzle } from "./puzzlesSlice"; | ||
import { hideModal } from "./modalSlice"; | ||
import { selectHuntTags } from "./huntSlice"; | ||
|
||
function AddPuzzleModal({ huntId }) { | ||
const allTags = useSelector(selectHuntTags); | ||
const [name, setName] = React.useState(""); | ||
const [url, setUrl] = React.useState(""); | ||
const [assignedMeta, setAssignedMeta] = React.useState("none"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's wrong with using a falsy value for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nothing. I'll change this value and the default option value to "" |
||
const [isMeta, setIsMeta] = React.useState(false); | ||
const [createChannels, setCreateChannels] = React.useState(true); | ||
const dispatch = useDispatch(); | ||
|
@@ -21,12 +24,14 @@ function AddPuzzleModal({ huntId }) { | |
url, | ||
is_meta: isMeta, | ||
create_channels: createChannels, | ||
assigned_meta: assignedMeta, | ||
}) | ||
).finally(() => { | ||
dispatch(hideModal()); | ||
}); | ||
return false; | ||
}; | ||
|
||
return ( | ||
<> | ||
<Modal.Header closeButton> | ||
|
@@ -53,6 +58,25 @@ function AddPuzzleModal({ huntId }) { | |
onChange={(e) => setUrl(e.target.value)} | ||
/> | ||
</Form.Group> | ||
<Form.Group controlId="addPuzzle.meta"> | ||
<Form.Label>Assigned Meta</Form.Label> | ||
<Form.Control | ||
as="select" | ||
value={assignedMeta} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you use a falsy value like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll just change both to "". |
||
onChange={(e) => setAssignedMeta(e.target.value)} | ||
> | ||
<option key="none" value="none"> | ||
None | ||
</option> | ||
{allTags | ||
.filter((tag) => tag.is_meta) | ||
.map((tag, i) => ( | ||
<option key={tag.name} value={tag.name}> | ||
{tag.name} | ||
</option> | ||
))} | ||
</Form.Control> | ||
</Form.Group> | ||
<Form.Check | ||
type="checkbox" | ||
label="Is meta" | ||
|
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.
Should we add a unit test for this? (Can be in a separate PR if we want to expedite this 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.
Sure. I'll add a unit test to this PR