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.

Sign up for GitHub

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

Jump to bottom

test(flow-item, panel): add tests for internal property. #7931

Merged
merged 6 commits into from
Oct 5, 2023
Merged

test(flow-item, panel): add tests for internal property. #7931

merged 6 commits into from
Oct 5, 2023

Conversation

driskull
Copy link
Member

@driskull driskull commented Oct 3, 2023 •
edited
Loading

Related Issue: #7930

Summary

  • Adds stories for internal collapseDirection property.

@driskull driskull marked this pull request as ready for review October 3, 2023 23:13
@driskull driskull requested a review from a team as a code owner October 3, 2023 23:13
@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Oct 3, 2023
@driskull driskull added the low risk Issues with low risk for consideration in low risk milestones label Oct 3, 2023
jcfranco
jcfranco reviewed Oct 4, 2023
View reviewed changes
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

If this prop only affects the icon and not the way the panel expands, for consistency, could we rename it to follow something similar to flipRtl?

packages/calcite-components/src/components/flow-item/flow-item.stories.ts Outdated
@@ -159,6 +167,10 @@ export const collapsed_TestOnly = (): string => html`
<calcite-flow-item collapsed collapsible closable> Hello World! </calcite-flow-item>
`;

export const collapsedInverted_TestOnly = (): string => html`
Copy link
Member

Choose a reason for hiding this comment

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

collapsedInverted_TestOnly ➡️ collapseInverted_TestOnly

@driskull
Copy link
Member Author

driskull commented Oct 5, 2023

If this prop only affects the icon and not the way the panel expands, for consistency, could we rename it to follow something similar to flipRtl?

@jcfranco I had considered flipRTL but it actually doesn't follow our naming standard. Ideally, it would be rtl-flipped to be more inline with how HTML names attributes right? Do we want to consider updating that property to be more inline with our other props?

@driskull driskull requested a review from jcfranco October 5, 2023 16:05
@macandcheese
Copy link
Contributor

Since ‘collapseInverted’ doesn’t really explain what the default direction is, maybe ‘collapseDirection’ top / bottom with a meaningful default is easier to understand?

It does seem like it would maybe be confusing or lead to misuse as an option in Block, Accordion, etc, so would this only be available in Panel?

@driskull
Copy link
Member Author

driskull commented Oct 5, 2023

Since ‘collapseInverted’ doesn’t really explain what the default direction is, maybe ‘collapseDirection’ top / bottom with a meaningful default is easier to understand?

I agree. Maybe you me and @jcfranco can figure out a name? I can also just make this internal for now.

It does seem like it would maybe be confusing or lead to misuse as an option in Block, Accordion, etc, so would this only be available in Panel?

I think it could be useful in anything that uses expand arrows to indicate expand direction. Currently, we assume that accordion and block open below but if the component is anchored at the bottom of something then that isn't the case anymore.

@jcfranco
Copy link
Member

jcfranco commented Oct 5, 2023

Ideally, it would be rtl-flipped to be more inline with how HTML names attributes right? Do we want to consider updating that property to be more inline with our other props?

Good point. I don't think we focus on this explicitly in our conventions. From what I recall, we've suggested concise adjectives for boolean names. We could update the conventions and do another audit on boolean names.

FWIW, I was more suggesting to use flip in the name. 😅

Since ‘collapseInverted’ doesn’t really explain what the default direction is, maybe ‘collapseDirection’ top / bottom with a meaningful default is easier to understand?

Agreed. Would we ever have other directions?

I'm fine with keeping it internal while we hash this out to land the increased test coverage.

jcfranco
jcfranco approved these changes Oct 5, 2023
View reviewed changes
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Aside from the naming discussion, LGTM!

Good to merge as is if the prop is made internal.

@macandcheese
Copy link
Contributor

macandcheese commented Oct 5, 2023 •
edited
Loading

I think it could be useful in anything that uses expand arrows to indicate expand direction. Currently, we assume that accordion and block open below but if the component is anchored at the bottom of something then that isn't the case anymore.

Yeah, although I think flipping the direction of the icon is unneeded because the area that expands is still opening in the same space / direction - the containing element just gets taller. A "real flip" IMO would be the content area opening on the "other side" of the element. That said, it can be confused with a "collapse further or dock at bottom" in the case of the JSAPI example, and I know we didn't want to flip the position of the title bar just to facilitate a more literal chevron meaning, lol.

@driskull driskull added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Oct 5, 2023
@driskull driskull changed the title feat(flow-item, panel): add collapseInverted property to flip the collapse icon test(flow-item, panel): add tests for internal property. Oct 5, 2023
@driskull
Copy link
Member Author

driskull commented Oct 5, 2023

Reverted to previous name and @internal state.

@driskull
Copy link
Member Author

driskull commented Oct 5, 2023

@ashetland can you review screenshot tests?

@driskull driskull added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 5, 2023
@github-actions github-actions bot added the testing Issues related to automated or manual testing. label Oct 5, 2023
@driskull driskull added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 5, 2023
@driskull driskull merged commit 7ad1ce0 into main Oct 5, 2023
14 checks passed
@driskull driskull deleted the dris0000/collapse-inverted-prop branch October 5, 2023 19:00
benelan added a commit that referenced this pull request Oct 11, 2023
@benelan
Merge remote-tracking branch 'origin/main' into astump/chore-add-configs
0fe870a 
* origin/main: (83 commits)
  chore: add design tokens to monorepo (#7948)
  test(input-time-zone): restore common tests (#7963)
  chore: release next
  fix(input-time-zone): fix error caused by time zone group filtering (#7971)
  chore: release next
  fix(table): Improve scrollbar display (#7967)
  build: bump timezone-groups (#7962)
  chore: release next
  fix(flow-item): update collapsed property when collapse button is clicked (#7960)
  chore: release main (#7945)
  chore: release next
  fix(input-time-zone): fix handling of unknown and cityless time zones from offset display mode (#7947)
  fix(combobox): fix issue causing value to be cleared when selecting an item (Windows + trackpad) (#7954)
  chore(deps): update calcite-ui-icons to 3.24.7 (#7949)
  test(flow-item, panel): add tests for internal property. (#7931)
  chore: release next
  fix(panel): fix collapse action title and reverse icon direction (#7927)
  chore: update issue template labels to support workflows (#7938)
  build(deps): bump timezone-groups (#7933)
  build(deps): update dependency @esri/calcite-ui-icons to v3.24.6 (#7925)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alisonailea alisonailea alisonailea approved these changes

@jcfranco jcfranco jcfranco approved these changes

Assignees

@driskull driskull

Labels
enhancement Issues tied to a new feature or request. low risk Issues with low risk for consideration in low risk milestones pr ready for visual snapshots Adding this label will run visual snapshot testing. testing Issues related to automated or manual testing.
Projects
None yet
Milestone
2023 October Priorities - Maintenance...
Development

Successfully merging this pull request may close these issues.
4 participants
@driskull @macandcheese @jcfranco @alisonailea