fix(accordion): expand/collapse through isOpen prop #775

IuriPires · 2023-05-27T17:02:08Z

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

It fixes #735

Fixes # (735)

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change contains documentation update

Breaking changes

Please document the breaking changes if suitable.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Test A
Test B

Test Configuration:

Firmware version:
Hardware:
Toolchain:
SDK:

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules
I have checked my code and corrected any misspellings

vercel · 2023-05-27T17:02:12Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
flowbite-react	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 16, 2023 8:56am

IuriPires · 2023-05-27T17:09:07Z

@rluders , I worked on this one but now I'm thinking: seems that Flowbite-React never wanted to expose isOpen and setOpen properties directly, based on what I saw in the code. Maybe this one can be treated as a feature instead of a bug? Up to discussion.

IuriPires · 2023-05-27T17:12:30Z

Marked as bug and feature. @tulup-conner , what you think about this one?

tulup-conner

Please refer to the contributing guide. Specifically, you need to make sure your PR passes the CI before submitting for review. It looks like you are not using prettier or are with a custom config. In the future, simply running

yarn format && yarn lint && yarn test && yarn build

Before you submit your changes will save a lot of time.

The changes look good overall you just did some weird stuff with Sidebar for some reason that fails the formatting check. Just undo that and the other stuff I mentioned and we will test this out.

When the CI checks pass and you've made the aforementioned changes just set this to Ready for review or, if you can't do that (I'm not sure) feel free to mention me again and I will check it out

tulup-conner · 2023-05-31T00:40:32Z

src/components/Accordion/Accordion.spec.tsx

@@ -92,7 +92,6 @@ describe('Components / Accordion', () => {
      render(
        <>
          <TestAccordion />
-          {/* eslint-disable-next-line jsx-a11y/role-has-required-aria-props */}


Have you run yarn lint to confirm this works? The comment is there for a reason - <button>s are only supposed to have role="button". This test should just be rewritten to make this ESLint warning not come up in the first place but, otherwise, you'll need to restore this line.

tulup-conner · 2023-05-31T00:43:48Z

src/components/Sidebar/index.ts

 export type { SidebarCollapseProps } from './SidebarCollapse';
+export type { SidebarCTAProps } from './SidebarCTA';


I'm confused if you have a custom prettier config somehow or have just made these changes of your own accord, but they fail yarn prettier so you need to revert them. I'm not sure why they were made anyway in a PR for the Accordion

tulup-conner · 2023-05-31T00:45:32Z

src/components/Accordion/Accordion.stories.tsx

@@ -1,4 +1,4 @@
-import type { Meta, StoryFn } from '@storybook/react';
+import type { Meta, Story } from '@storybook/react/types-6-0';


Please restore this change, see storybookjs/storybook#14190

tulup-conner · 2023-05-31T00:46:14Z

src/components/Accordion/Accordion.spec.tsx

@@ -266,15 +265,31 @@ describe('Components / Accordion', () => {
      expect(content()[1]).not.toBeVisible(); // content should not be visible
    });
  });
+
+  describe('Collapse/expand panel', () => {


Please just collapse this into a single test like

should collapse panel if isOpen and expand if not after a user toggle event

Instead of introducing a beforeEach here

IuriPires · 2023-06-09T10:08:24Z

Thank you @tulup-conner and sorry for replying after so long. Was kinda busy during last weeks. I checked and you were right. I had some custom configs on my VSCode. I have also analysed every point you've mentioned.

codecov · 2023-06-09T10:11:11Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (470c359) 99.54% compared to head (35990cd) 99.54%.

❗ Current head 35990cd differs from pull request most recent head eae1d7c. Consider uploading reports for the commit eae1d7c to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #775      +/-   ##
==========================================
- Coverage   99.54%   99.54%   -0.01%     
==========================================
  Files         129      129              
  Lines        6539     6538       -1     
  Branches      391      392       +1     
==========================================
- Hits         6509     6508       -1     
  Misses         30       30

Impacted Files	Coverage Δ
src/components/Accordion/Accordion.tsx	`100.00% <100.00%> (ø)`
src/components/Accordion/AccordionPanel.tsx	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

IuriPires · 2023-06-09T10:15:31Z

@tulup-conner also sorry for bad commiting type. I will be more precise and I'll re-read again the contribution guide to give the best following the contribuition rules.

rluders · 2023-06-11T14:58:45Z

@IuriPires could you please, instead merge the main use rebase instead? It is better for history tracking on git.

IuriPires · 2023-06-12T21:38:41Z

@IuriPires could you please, instead merge the main use rebase instead? It is better for history tracking on git.

Done :)

rluders · 2023-06-16T11:01:44Z

@IuriPires I think that it isn't actually working as expected, or there is something wrong, check it here: https://flowbite-react-3hzdjhhhs-themesberg.vercel.app/docs/components/accordion (ps: also the docs sidebar isn't working after the change)

IuriPires · 2023-06-16T11:16:31Z

@IuriPires I think that it isn't actually working as expected, or there is something wrong, check it here: https://flowbite-react-3hzdjhhhs-themesberg.vercel.app/docs/components/accordion (ps: also the docs sidebar isn't working after the change)

Hi there @rluders , could you confirm a thing? Main isn't work on my local env. I'm currently using node v18.15.0 (not building/serving Storybook) could you confirm this please? I've checkout older branches and it worked fine.

This is main:

This is a 3 week old branch

rluders · 2023-06-29T13:55:11Z

Sorry, @IuriPires just had time to reply to you now. So... Not sure what is happening. I'm using node v19.2.0 and I don't have any issues with the storybook.

rluders · 2023-07-04T12:17:58Z

@IuriPires I think that this one just needs a rebase, right?

rluders · 2023-07-04T12:18:21Z

@IuriPires I think that it isn't actually working as expected, or there is something wrong, check it here: https://flowbite-react-3hzdjhhhs-themesberg.vercel.app/docs/components/accordion (ps: also the docs sidebar isn't working after the change)

Or maybe not... 🤔

dima-kov · 2023-12-22T16:21:40Z

any progress on this?

vercel bot temporarily deployed to Preview May 27, 2023 17:03 Inactive

vercel bot temporarily deployed to Preview May 27, 2023 17:07 Inactive

vercel bot temporarily deployed to Preview May 27, 2023 17:15 Inactive

tulup-conner requested changes May 31, 2023

View reviewed changes

tulup-conner marked this pull request as draft May 31, 2023 00:49

vercel bot temporarily deployed to Preview June 9, 2023 09:32 Inactive

IuriPires force-pushed the 735 branch from a85668e to 45cc426 Compare June 9, 2023 10:05

vercel bot temporarily deployed to Preview June 9, 2023 10:08 Inactive

IuriPires requested a review from tulup-conner June 9, 2023 10:12

IuriPires marked this pull request as ready for review June 11, 2023 14:12

vercel bot temporarily deployed to Preview June 11, 2023 14:15 Inactive

IuriPires force-pushed the 735 branch from 35990cd to 25f229b Compare June 12, 2023 21:38

vercel bot temporarily deployed to Preview June 12, 2023 21:41 Inactive

IuriPires added 7 commits June 16, 2023 10:53

fix: expand/collapse through isOpen prop

1e5ee9b

fix: expand/collapse through isOpen prop

04c9bee

chore: prettier fix

f180853

style: prettier

c8aca31

test: improve test case

f58d28a

fix: rollback previous file

78038a7

refactor: remove unnecessary export

eae1d7c

rluders force-pushed the 735 branch from 25f229b to eae1d7c Compare June 16, 2023 08:53

vercel bot temporarily deployed to Preview June 16, 2023 08:56 Inactive

rluders changed the title ~~fix: expand/collapse through isOpen prop~~ fix(accordion): expand/collapse through isOpen prop Jul 16, 2023

rluders marked this pull request as draft July 18, 2023 17:11

		export type { SidebarCollapseProps } from './SidebarCollapse';
		export type { SidebarCTAProps } from './SidebarCTA';

		@@ -1,4 +1,4 @@
		import type { Meta, StoryFn } from '@storybook/react';
		import type { Meta, Story } from '@storybook/react/types-6-0';

Uh oh!

fix(accordion): expand/collapse through isOpen prop #775

Are you sure you want to change the base?

fix(accordion): expand/collapse through isOpen prop #775

Uh oh!

Conversation

IuriPires commented May 27, 2023

Description

Type of change

Breaking changes

How Has This Been Tested?

Checklist:

Uh oh!

vercel bot commented May 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IuriPires commented May 27, 2023

Uh oh!

IuriPires commented May 27, 2023

Uh oh!

tulup-conner left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tulup-conner May 31, 2023

Choose a reason for hiding this comment

Uh oh!

tulup-conner May 31, 2023

Choose a reason for hiding this comment

Uh oh!

tulup-conner May 31, 2023

Choose a reason for hiding this comment

Uh oh!

tulup-conner May 31, 2023

Choose a reason for hiding this comment

Uh oh!

IuriPires commented Jun 9, 2023

Uh oh!

codecov bot commented Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

IuriPires commented Jun 9, 2023

Uh oh!

rluders commented Jun 11, 2023

Uh oh!

IuriPires commented Jun 12, 2023

Uh oh!

rluders commented Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IuriPires commented Jun 16, 2023

Uh oh!

rluders commented Jun 29, 2023

Uh oh!

rluders commented Jul 4, 2023

Uh oh!

rluders commented Jul 4, 2023

Uh oh!

dima-kov commented Dec 22, 2023

Uh oh!

Uh oh!

vercel bot commented May 27, 2023 •

edited

Loading

tulup-conner left a comment •

edited

Loading

codecov bot commented Jun 9, 2023 •

edited

Loading

rluders commented Jun 16, 2023 •

edited

Loading