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

Refactor interaction state solution for hover #2059

Conversation

jkaltoft
Copy link
Collaborator

@jkaltoft jkaltoft commented Feb 21, 2022

Which issue does this PR close?

This PR closes #2008

What is the new behavior?

Introduces layer based solution for interaction states in general and for hover state in particular.

Does this PR introduce a breaking change?

  • Yes
  • No

Are there any additional context?

Checklist:

The following tasks should be carried out in sequence in order to follow the process of contributing correctly.

Reminders

  • Make sure you have implemented tests following the guidelines in: "The good: Test".
  • Make sure you have updated the cookbook with examples and showcases (for bug fixes, enhancements & new components).

Review

  • Do a self-review.
  • Request that the changes are code-reviewed
  • Request that the changes are UX reviewed (only necessary if your PR introduces visual changes)

When the pull request has been approved it will be automatically merged to master via automerge.

@github-actions github-actions bot temporarily deployed to pr-2008-refactor-interaction-state-solution February 21, 2022 16:16 Inactive
@jkaltoft jkaltoft changed the title Enhancement/2008 refactor interaction state solution Enhancement #2008 - Refactor interaction state solution Feb 22, 2022
@jkaltoft jkaltoft changed the title Enhancement #2008 - Refactor interaction state solution Enhancement: Refactor interaction state solution Feb 22, 2022
@jkaltoft jkaltoft force-pushed the enhancement/2008-refactor-interaction-state-solution branch from 72b4de7 to ac4eee4 Compare February 22, 2022 11:09
@github-actions github-actions bot temporarily deployed to pr-2008-refactor-interaction-state-solution February 22, 2022 11:12 Inactive
@github-actions github-actions bot temporarily deployed to pr-2008-refactor-interaction-state-solution February 22, 2022 11:14 Inactive
@github-actions github-actions bot temporarily deployed to pr-2008-refactor-interaction-state-solution February 22, 2022 13:53 Inactive
@github-actions github-actions bot temporarily deployed to pr-2008-refactor-interaction-state-solution February 23, 2022 11:04 Inactive
@github-actions github-actions bot temporarily deployed to pr-2008-refactor-interaction-state-solution February 23, 2022 14:53 Inactive
@jkaltoft jkaltoft force-pushed the enhancement/2008-refactor-interaction-state-solution branch 2 times, most recently from 4d1f350 to 3ebb347 Compare February 23, 2022 15:14
@github-actions github-actions bot temporarily deployed to pr-2008-refactor-interaction-state-solution February 23, 2022 15:19 Inactive
@github-actions github-actions bot temporarily deployed to pr-2008-refactor-interaction-state-solution February 23, 2022 15:40 Inactive
@github-actions github-actions bot temporarily deployed to pr-2008-refactor-interaction-state-solution February 24, 2022 09:51 Inactive
@jkaltoft jkaltoft force-pushed the enhancement/2008-refactor-interaction-state-solution branch from 28e1939 to 9a9584d Compare February 24, 2022 09:57
@github-actions github-actions bot temporarily deployed to pr-2008-refactor-interaction-state-solution February 24, 2022 10:02 Inactive
@github-actions github-actions bot temporarily deployed to pr-2008-refactor-interaction-state-solution February 24, 2022 11:19 Inactive
@github-actions github-actions bot temporarily deployed to pr-2008-refactor-interaction-state-solution February 24, 2022 15:32 Inactive
@jkaltoft jkaltoft force-pushed the enhancement/2008-refactor-interaction-state-solution branch from 07db585 to ae20555 Compare February 25, 2022 15:18
@github-actions github-actions bot temporarily deployed to pr-2008-refactor-interaction-state-solution February 25, 2022 15:23 Inactive
@github-actions github-actions bot temporarily deployed to pr-2008-refactor-interaction-state-solution February 28, 2022 12:26 Inactive
@github-actions github-actions bot temporarily deployed to pr-2008-refactor-interaction-state-solution February 28, 2022 15:37 Inactive
@jkaltoft jkaltoft marked this pull request as ready for review February 28, 2022 15:44
@github-actions github-actions bot temporarily deployed to pr-2008-refactor-interaction-state-solution March 1, 2022 11:40 Inactive
@github-actions github-actions bot temporarily deployed to pr-2008-refactor-interaction-state-solution March 2, 2022 09:59 Inactive
@github-actions github-actions bot temporarily deployed to pr-2008-refactor-interaction-state-solution March 2, 2022 10:19 Inactive
@github-actions github-actions bot temporarily deployed to pr-2008-refactor-interaction-state-solution March 2, 2022 11:07 Inactive
@github-actions github-actions bot temporarily deployed to pr-2008-refactor-interaction-state-solution March 7, 2022 14:59 Inactive
Copy link
Contributor

@MadsBuchmann MadsBuchmann left a comment

Choose a reason for hiding this comment

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

Looking good - i got some comments 😄

Comment on lines 15 to 22
// NOTE: Also see ion-back-button in page component and ion-segment-button in segmented control component
&::part(native)::after {
@include ionic.transition;
}
// end NOTE
Copy link
Contributor

Choose a reason for hiding this comment

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

What should i note when looking at ion-back-button in page component and ion-segment-button in segmented control component? Consider elaborating.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we could add hyperlinks to code it would be great. The note is just an attempt to refer to the other places we use the same custom implementation of hover.

I hope we can consolidate this somehow when we finish the remaining issues for hover, where we need special solutions for different ways to make it work for Ionic components.

Copy link
Contributor

@MadsBuchmann MadsBuchmann Mar 10, 2022

Choose a reason for hiding this comment

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

Makes sense! I confused this with the shell comment... So i accidentally took a stab at reformulating it. If we consolidate them in the end it does not matter anyways :P

Suggested change
// NOTE: Also see ion-back-button in page component and ion-segment-button in segmented control component
&::part(native)::after {
@include ionic.transition;
}
// end NOTE
/*
NOTE: this is a custom implementation of the hover interaction state;
identical to PageComponent's ion-back-button & SegmentedControlComponent's
ion-segment-button.
*/
&::part(native)::after {
@include ionic.transition;
}
// end NOTE

@jkaltoft jkaltoft force-pushed the enhancement/2008-refactor-interaction-state-solution branch from 5be4067 to b267f24 Compare March 14, 2022 14:44
Copy link
Contributor

@MadsBuchmann MadsBuchmann left a comment

Choose a reason for hiding this comment

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

LGTM!

@jkaltoft jkaltoft changed the title Enhancement: Refactor interaction state solution Refactor interaction state solution for hover Mar 15, 2022
@jkaltoft jkaltoft merged commit 9ca753e into release/theme/2028-button-attention-levels-and-hover Mar 15, 2022
@jkaltoft jkaltoft deleted the enhancement/2008-refactor-interaction-state-solution branch March 15, 2022 12:29
jkaltoft added a commit that referenced this pull request Mar 17, 2022
* 💄 Fine-tune loudness levels for different cases

* 💄 Handle transparent background in _button-hover

* 💄 Use undecorated kirby-buttons in calendar

* 💄 Adjust hover on buttons in dark context

* 💄 Reduce attention level 3 hover loudness

* 💄 Increase alert ok button hover loudness

* 💄 Adjust hover loudness for action sheet button

* 💄 Remove margin from buttons in calendar

* 🚧 stash commit

* 🚧 Make PoC with state layer

* ⚗️ Try to use state layer in calendar

* ⚗️ Continue experiment with state layer

* 💄 Fix icon positioning in buttons

* 🚧 separate hover styles from generic layer styles

* 🔥 Do not use parenthesis with argument-less mixins

* 🚧 separate button specific styles for content from generic layer styles

* 💡 add todo comments

* 🔥 Do not use parenthesis with argument-less mixins

* 💡 Add comments

* 🔥 Clean up

* ♻️ Extract generic interaction state layer styles

* 🚚 Rename mixins

* ♻️ Extract state layer styles for all states

* 🔥 Clean up

* ♻️ Rename module

* 💄 Apply hover for ionic components (item)

* ✅ Update tests to match new template structure

* 💄 Use loudness names as parameters

* 💄 Hide apply-state mixin in module

* 🔥 Clean up

* 💄 Move state layer on top of content layer

* 💄 Use kirby-black as state layer bg color

* 💄 Make z-index for layers configurable

* 💡 Update/delete comments

* 💄 Move transition declaration into private mixin

* 💄 Rename class selector and clean up

* 🔥 Remove --kirby-item-background-hover

* 💄 Reorder declarations and remove comments

* 💄 Apply hover to ion-back-button

* Make transition mixin public
* Expose transition mixin from ionic module

* ♿️  Hide state layer from screen readers

* ♻️ move theme-background mixin

* 💄 Apply state layer to chip component

- Eliminates selected-and-hover() and theme-background() mixins
- Apply hover state layer where needed
- Declare color and background-color where needed (no mixin)

* 💄 Apply state layer to segmented control

* 📝 Update comment

* 🔥 Move shared styles to only place they're used

* 💄 Add content block in apply-hover mixin

* 💄 Apply proprietary hover solution to slide button

* 💄 Declare custom properties before regular properties

* 💄 Streamline special solutions for ionic buttons

- ion-back-button in Page component
- ion-segment-button in Segmented Control component

* 💄 Apply proprietary solution to ion-fab-button

* 💄 Adjust loudness levels and colors on Chip

* 💄 Update loudness scale - use t-shirt sizes

* 💡 Add/remove comments

* 💄 Align absence of named parameter for loudness

* 💄 Rename interaction state content layer

* 💄 Add interaction state layer and hover to Accordion

* ✅ Update tests

* 💄 Reintroduce default loudness variable

* 💡 Explain why we use negative inset value

* 💄 Remove redundant fallback value

* 💡 Remove comment

* 💄 Rename mixin and pass border-width as parameter

* 💡 Update comments

Co-authored-by: Mads Buchmann Frederiksen <mbf@prochimp.dk>

* 💡 Link to Github issue in comment

* 💄 Use correct import paths

* 💄 Order @forward declarations alphabetically

Co-authored-by: Mads Buchmann Frederiksen <mbf@prochimp.dk>
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