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

UIP-2751 Transition in/out-specific config, test attributes #120

Merged
merged 6 commits into from
Oct 17, 2017

Conversation

greglittlefield-wf
Copy link
Contributor

@greglittlefield-wf greglittlefield-wf commented Oct 6, 2017

Ultimate problem:

  • There wasn't a way to configure the number of transitions to be different between showing and hiding
  • We want to add test attributes that mirror the state of the transition in the DOM in consistent way

How it was fixed:

  • Add transitionInCount/transitionOutCount props and update AbstractTransitionComponent to support them
  • Add AbstractTransitionComponent.getTransitionTestAttributes for use in consumer implementations

Testing suggestions:

Potential areas of regression:

  • AbstractTransition

FYA: @greglittlefield-wf @aaronlademann-wf @jacehensley-wf @clairesarsam-wf @joelleibow-wf

@aviary-wf
Copy link

aviary-wf commented Oct 6, 2017

Raven

Number of Findings: 0

@codecov-io
Copy link

codecov-io commented Oct 6, 2017

Codecov Report

Merging #120 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #120      +/-   ##
==========================================
+ Coverage   94.39%   94.42%   +0.04%     
==========================================
  Files          31       31              
  Lines        1549     1559      +10     
==========================================
+ Hits         1462     1472      +10     
  Misses         87       87

@rmconsole-wf rmconsole-wf changed the title Transition in/out-specific config, test attributes UIP-2751 Transition in/out-specific config, test attributes Oct 6, 2017

if (props.transitionCount == 0) {
warningMessage += ' Instead of setting this prop to 0, override the `hasTransition` getter to return false.';
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are no longer necessary; transition counts that are <=0 are accounted for in hasTransitionIn and hasTransitionOut, and behave as if there is no transition (see tests).

Map<String, String> getTransitionTestAttributes() {
if (!component_base.UiProps.testMode) return const {};

const enumToAttrValue = const <TransitionPhase, String>{
Copy link
Contributor

@seangerhardt-wf seangerhardt-wf Oct 9, 2017

Choose a reason for hiding this comment

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

Noobie question - what is the attribute text for the "TransitionPhase"? I assume this is the attribute I'll have to search for in my functional tests when checking if the modal dialog has finished animating.

Copy link
Contributor

Choose a reason for hiding this comment

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

@seangerhardt-wf the HTML will look like this based on the current transition phase (only listing the ones you'd probably be interested in for functional test purposes):

TransitionPhase.SHOWN

<div data-transition-phase="shown"><!-- ... -->

TransitionPhase.HIDDEN

<div data-transition-phase="hidden"><!-- ... -->

Copy link
Contributor

@aaronlademann-wf aaronlademann-wf left a comment

Choose a reason for hiding this comment

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

One question / suggestion @greglittlefield-wf - otherwise looks great.

};

return {
'data-transition-phase': enumToAttrValue[state.transitionPhase],
Copy link
Contributor

Choose a reason for hiding this comment

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

@greglittlefield-wf should we store data-transition-phase as a constant sorta like we do for the test id attribute?

@jacehensley-wf
Copy link
Contributor

+1

@jacehensley-wf
Copy link
Contributor

+1

Necessary to support existing consumers who don’t
properly add inherited default props.
@clairesarsam-wf
Copy link
Contributor

+1

@jacehensley-wf
Copy link
Contributor

QA +10

  • Build successful
  • tests pass
  • WSD build passes

  • Testing instruction
  • Dev +1's
  • Unit tests created/updated
  • All unit tests pass
  • Rosie ran/Rosie comment displays expected info
  • Dependency Scan Clean

Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants