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

Work out audio upload issues #364

Merged
merged 8 commits into from
Jan 30, 2024
Merged

Work out audio upload issues #364

merged 8 commits into from
Jan 30, 2024

Conversation

CharlieMcVicker
Copy link
Collaborator

@CharlieMcVicker CharlieMcVicker commented Nov 17, 2023

  • fix backend weirdness related to merge?
  • cleanup contributor audio editing
  • prettier

TODO

  • Some AWS stuff is wonky: Uploads to S3 fail with NotAuthorizedException: Token is not from a supported provider of this identity pool.
  • The styles for this should be refactored
  • It's possible contributors.tsx needs to be broken up

- fix broken upload button
- provide visual feedback on upload status, failure
Copy link

netlify bot commented Nov 17, 2023

Deploy Preview for dailp ready!

Name Link
🔨 Latest commit ad6a1c8
🔍 Latest deploy log https://app.netlify.com/sites/dailp/deploys/65b7c5b8a69cb90008aa36f9
😎 Deploy Preview https://deploy-preview-364--dailp.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@CharlieMcVicker CharlieMcVicker marked this pull request as draft November 17, 2023 21:00
Copy link
Collaborator Author

@CharlieMcVicker CharlieMcVicker left a comment

Choose a reason for hiding this comment

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

Roadmap of changes

Comment on lines 136 to 200
<div
<form
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure this change was actually needed

Comment on lines 143 to 220
<label htmlFor="file-upload" style={{ display: "inline-block" }}>
<div style={{ display: "inline-block" }}>
<IconTextButton
icon={<MdUploadFile />}
// @ts-ignore -- I couldn't find a way to fix how this is typed
// this is part of React's ugly insides
as="label"
htmlFor="file-upload"
className={
currentTab === "upload" ? subtleButtonActive : subtleButton
}
>
Upload audio
</IconTextButton>
</label>
</div>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There was some bad interaction happening between the button and label elements here. This seems to have resolved the issue, kept the tab order correct, and kept cursor changes correct as well.

Comment on lines 271 to 284
<div
style={{
position: "absolute",
top: 0,
left: 0,
bottom: 0,
right: 0,
background: "rgba(255,255,255,0.65)",
color: "black",
padding: 40,
backdropFilter: "blur(2px)",
border: `4px solid ${border}`,
}}
>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should live in a ...styles.tsx file

Copy link
Contributor

Choose a reason for hiding this comment

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

Please make this change

Comment on lines 84 to 85
const wordEmbedStyle = /\[(\w*):([0-9]*)-?([0-9]*)?:?(audio)?(join)?(#)?(\w*)?\]/ // [DocName:Start(-OptionalEnd):?(audio?)(join?)#?OptionalChapterSlug?]
const wordEmbedStyle =
/\[(\w*):([0-9]*)-?([0-9]*)?:?(audio)?(join)?(#)?(\w*)?\]/ // [DocName:Start(-OptionalEnd):?(audio?)(join?)#?OptionalChapterSlug?]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@GracefulLemming we may be in a formatting battle - my dev-check changed this, but the last edit was you and the commit message was prettier

Copy link
Contributor

Choose a reason for hiding this comment

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

try merging main in– running prettier -w src and prettier -w --config package.json src yields two different outputs. All of our checks use the one with --config specified so thats likely what I ran

@CharlieMcVicker CharlieMcVicker changed the title cm/cleanup audio upload [WIP] Work out audio upload issues Nov 18, 2023
@GracefulLemming
Copy link
Contributor

"us-east-1:6d544733-83e2-4d38-baa3-195d3bfdf54b" # FIXME this should be causing some weird behavior somewhere...

Ah... fixing this in a new branch now


async function uploadAudioAndReset(data: Blob) {
const success = await uploadAudio(data)
if (success) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we notify the user if this fails, or at least log an error in the console?

Comment on lines 271 to 284
<div
style={{
position: "absolute",
top: 0,
left: 0,
bottom: 0,
right: 0,
background: "rgba(255,255,255,0.65)",
color: "black",
padding: 40,
backdropFilter: "blur(2px)",
border: `4px solid ${border}`,
}}
>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make this change

@CharlieMcVicker CharlieMcVicker changed the title [WIP] Work out audio upload issues Work out audio upload issues Jan 29, 2024
@@ -323,5 +418,5 @@ async function uploadContributorAudioToS3(user: CognitoUser, data: Blob) {
})
)

return { resourceUrl: `https://d1q0qkah8ttfau.cloudfront.net/${key}` }
return { resourceUrl: `https://${process.env["CF_URL"]}/${key}` }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does the cloudflare url have https:// at the front or no? I found some code from @GracefulLemming from four months ago where we started prepending https:// in a backend process. I'm struggling to find where this secret is managed.

@GracefulLemming GracefulLemming merged commit 1c3d93e into main Jan 30, 2024
5 checks passed
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