Skip to content

Add audio context file #1

Open
KageJordan wants to merge 9 commits into
mainfrom
KageJordan-patch-1
Open

Add audio context file #1
KageJordan wants to merge 9 commits into
mainfrom
KageJordan-patch-1

Conversation

@KageJordan

Copy link
Copy Markdown
Owner

No description provided.

@KageJordan KageJordan self-assigned this Nov 14, 2022
@KageJordan KageJordan requested a review from snuggs November 14, 2022 23:31

@snuggs snuggs left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A few nits. Looking good.

Comment thread audio_context.html Outdated
Comment thread audio_context.html Outdated
Comment thread audio_context.html Outdated
Comment thread audio_context.html Outdated
Comment thread audio_context.html Outdated
Comment thread audio_context.html Outdated
Comment thread audio_context.html Outdated
Comment thread audio_context.html Outdated
Adds bindings for `submit`, `formdata`, `change`, & `input` form events. Perhaps missing `cancel`?
@snuggs

snuggs commented Nov 18, 2022

Copy link
Copy Markdown
Collaborator

@KageJordan please review 👉 fd4422f

@snuggs snuggs left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A few nits

Comment thread audio_context.html Outdated
Comment thread audio_context.html Outdated
Comment thread audio_context.html Outdated

<fieldset>

<legend hidden>Checkboxes</legend>

@snuggs snuggs Nov 21, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

When you figure out a better legend text you want to keep that content by making it "invisible" not hidden (read comments). Fine for now to yank it out the dom but remember for later. Ultimately there should be no hiddens unless controlled by javascript. But fine to use during development. Just remember to remove before going live, like console.logs.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Sounds good ,I hadn't thought about using display:none.Gracias

Comment thread audio_context.html
<legend hidden>Checkboxes</legend>

<label>
<input name=pads type="checkbox">

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Interesting if you can make a convention around the value=.... Perhaps some convention like value=thunder then can use js to retrieve value and load thunder.mp3 or new ThunderCompressor etc.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

👍🏽

Comment thread audio_context.html Outdated
<input name=pads type="checkbox">
</label>

</fieldset>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Triple closing </fieldset>s on the same nesting level? 🤔

@KageJordan KageJordan Nov 22, 2022

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

It should be one < fieldset> with 4 nesting < fieldset>s within , might have created too many spaces between tags, could be confusing.

Comment thread audio_context.html Outdated
Comment thread audio_context.html Outdated
Comment thread audio_context.html
Comment thread audio_context.html Outdated

@snuggs snuggs left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM! Let's discuss next commits to see what's left for this PR/branch before merging. 🏆

Comment thread audio_context.html
Comment thread audio_context.html Outdated
Comment thread audio_context.html Outdated
Comment thread audio_context.html Outdated
Comment thread audio_context.html Outdated
Removed content from second <header >
Reordered content in <input > elements

@snuggs snuggs left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Pretty good. Basically splitting html hairs at this point.

Comment thread audio_context.html
<body>

<header>
<h1>DevPunks Sequencer</h1>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Off by 1 (whitespace) error? 😉

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