Skip to content

Conversation

@codingdesigner
Copy link

Brief Description

JIRA Link

Related Issue

General Notes

Motivation and Context

Issues and Limitations

Types of changes

  • Bug fix
  • New feature
  • Breaking change

Checklist

  • This PR adds or removes a Storybook story.
  • I have added tests to cover my changes.
  • Regressions are passing and/or failures are documented
  • Changes have been checked in evergreen browsers

@changeset-bot
Copy link

changeset-bot bot commented Oct 10, 2025

⚠️ No Changeset found

Latest commit: 799ad11

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@netlify
Copy link

netlify bot commented Oct 10, 2025

Deploy Preview for astro-stencil ready!

Name Link
🔨 Latest commit 799ad11
🔍 Latest deploy log https://app.netlify.com/projects/astro-stencil/deploys/68fbbdb472b0f000085c5fa7
😎 Deploy Preview https://deploy-preview-1427--astro-stencil.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 project configuration.

@netlify
Copy link

netlify bot commented Oct 10, 2025

Deploy Preview for astro-preview ready!

Name Link
🔨 Latest commit 799ad11
🔍 Latest deploy log https://app.netlify.com/projects/astro-preview/deploys/68fbbdb4be3ead00076a1174
😎 Deploy Preview https://deploy-preview-1427--astro-preview.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 project configuration.

@codingdesigner codingdesigner marked this pull request as draft October 10, 2025 19:03
Copy link
Contributor

@FMorrison87 FMorrison87 left a comment

Choose a reason for hiding this comment

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

This looks awesome. Very thorough!

}

// Topic-specific validation
if (this.activeTopic === 'issue' || this.activeTopic === 'bug') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would replace these if statements for activeTopic with a switch statement.


// Check if a form topic should be displayed
private isFormIncluded(topic: FeedbackTopic): boolean {
console.log('this.included_forms', topic, this.includedForms)
Copy link
Contributor

Choose a reason for hiding this comment

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

Stray log?

if (!this.uploadableTopics || this.uploadableTopics.trim() === '') {
return true // All topics allow uploads if not restricted
}
if (this.uploadableTopics.toLowerCase() === 'none') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor into else if?

// Handle file upload
private handleFileUpload = (event: Event) => {
const input = event.target as HTMLInputElement
const files = input.files
Copy link
Contributor

Choose a reason for hiding this comment

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

Would probably be good to assert what type this is to ensure the Array.from() later on is valid.

const dropZone = event.currentTarget as HTMLElement
dropZone?.classList.remove('drag-over')

const files = event.dataTransfer?.files
Copy link
Contributor

Choose a reason for hiding this comment

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

Assert the type for files.

}

// Add standard topics if included
if (this.isFormIncluded('issue')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Switch statement for these.

private renderSentimentButtons() {
return (
<div class="rux-form__sentiment-buttons" part="sentiment-group">
{this.renderSentimentButton('positive', '😊')}
Copy link
Contributor

Choose a reason for hiding this comment

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

lol

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.

3 participants