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

Accordion component #4080

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

CrowsVeldt
Copy link

@CrowsVeldt CrowsVeldt commented Jan 16, 2025

PR Checklist

PR Type

What kind of change does this PR introduce?

  • Bugfix (fixes an issue)
  • Feature (adds functionality)
  • Code style update
  • Refactoring (no functional changes)
  • Build related changes
  • CI related changes
  • Documentation changes
  • Other... Please describe:

What is the current behavior?

What is the new behavior?

Describe the new behaviour
If useful, provide screenshot or capture to highlight main changes

Does this PR introduce a breaking change?

  • Yes
  • No

Git Issues

Closes #3786

What happens next?

Thanks for the contribution! We try to make sure all PRs are reviewed ahead of our monthly maintainers call (first Monday of the month)

If the PR is working as intended it'll be merged and included in the next platform release, if not changes will be requested and re-reviewed once updated.

If you need more immediate feedback you can try reaching out on Discord in the Community Platform development channel.

@CrowsVeldt CrowsVeldt requested a review from a team as a code owner January 16, 2025 16:40
@CrowsVeldt
Copy link
Author

The 'Current Email' subtitle doesn't have the underline anymore. I wasn't sure how to add it without making a specific case just for the email form.

Copy link
Member

@benfurber benfurber left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks @CrowsVeldt. A few minor bits, plus you need to run yarn format.

} as Meta<typeof Accordion>

export const Default: StoryFn<typeof Accordion> = () => (
<Accordion title="Default">
Copy link
Member

Choose a reason for hiding this comment

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

It make this a little more meaningful, how's about the title of 'Accordion Title'


export const Default: StoryFn<typeof Accordion> = () => (
<Accordion title="Default">
<Text>Default</Text>
Copy link
Member

Choose a reason for hiding this comment

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

And change this to 'Now you see me!'

Comment on lines +12 to +17
const { getByText } = render(
<Default {...(Default.args as IProps)} />,
)

expect(getByText('Accordion')).toBeInTheDocument();
})
Copy link
Member

Choose a reason for hiding this comment

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

Probably something like this would be good:

  it('displays the accordion body on click', () => {
    const { getByText } = render(
      <Default {...(Default.args as IProps)} />,
    )
      const accordionTitle = getByText('Accordion Title')
      expect(getByText('Now you see me!')).not.toBeInTheDocument()

      accordionTitle.click()

      expect(getByText('Now you see me!')).toBeInTheDocument()

@@ -76,3 +76,4 @@ export { VideoPlayer } from './VideoPlayer/VideoPlayer'
export { CommentAvatar } from './CommentAvatar/CommentAvatar'
export type { availableGlyphs } from './Icon/types'
export type { ITab } from './SettingsFormWrapper/SettingsFormTab'
export { Accordion } from './Accordion/Accordion'
Copy link
Member

Choose a reason for hiding this comment

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

If you wanted to be really nice, could you put this in alphabetical order please. :)

@benfurber benfurber force-pushed the accordion-component branch from cc7f3d5 to d48ab53 Compare January 17, 2025 15:26
@benfurber benfurber added the Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview label Jan 17, 2025
@benfurber
Copy link
Member

@CrowsVeldt Just QA'd it.

  1. Can you please add a change of mouse cursor on hover, so it's clearer where users can click.
  2. Can you reorder them so that password change is first.
  3. Can you please add the subheading for the change password.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview
Projects
Status: 💬 Changes Requested/with author
Development

Successfully merging this pull request may close these issues.

feat: add accordion for settings form
2 participants