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

feat(Unification): Update Command Group UI #20465

Merged
merged 120 commits into from
Apr 27, 2022
Merged

Conversation

emilyrohrbough
Copy link
Member

@emilyrohrbough emilyrohrbough commented Mar 3, 2022

Command Group UI Requirements

  • When logging is enabled & a Group has one or more command/event logs, a log will display in the Command Log with the group label.
  • Must be able to expand/collapse to show/hide logs that occurred within the context.
  • Must be expanded by default in Run Mode to capture logs during test recording.
  • Must support "printing to the console" to provide the end-user with more details.
  • All errors/failures that occur in a group must be surfaced and displayed to the end-user regardless even if group has logging disabled.

User facing changelog

TODO

How has the user experience changed?

View Percy screenshots for the most comprehensive comparisons.

Command Group Behavior Before:

Command Group Behavior After:

Screen.Recording.2022-04-21.at.8.14.32.AM.mov

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • [n/a] Has a PR for user-facing changes been opened in cypress-documentation?
  • [n/a] Have API changes been updated in the type definitions?

@emilyrohrbough emilyrohrbough marked this pull request as ready for review April 21, 2022 15:06
@emilyrohrbough emilyrohrbough requested a review from a team as a code owner April 21, 2022 15:06
const level = model.groupLevel < 6 ? model.groupLevel : 5

for (let i = 1; i < level; i++) {
groupPlaceholder.push(<span key={`${this.props.groupId}-${level}`} className={`command-group-id-${model.group} command-group-block`} />)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be:

Suggested change
groupPlaceholder.push(<span key={`${this.props.groupId}-${level}`} className={`command-group-id-${model.group} command-group-block`} />)
groupPlaceholder.push(<span key={`${this.props.groupId}-${i}`} className={`command-group-id-${model.group} command-group-block`} />)

@@ -259,53 +265,61 @@ class Command extends Component<Props> {
const isSessionCommand = commandName === 'session'
const displayNumOfChildren = !isSystemEvent && !isSessionCommand && model.hasChildren && !model.isOpen

let groupPlaceholder: Array<JSX.Element> = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be a const right?

Suggested change
let groupPlaceholder: Array<JSX.Element> = []
const groupPlaceholder: Array<JSX.Element> = []

@@ -343,7 +357,7 @@ class Command extends Component<Props> {

return (
<ul className='command-child-container'>
{_.map(model.children, (child) => (
{model.children.map((child) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Is children guaranteed to be populated on model? I like the change to a standard map, but it was safe against undefined values before, just wanted to confirm.

Copy link
Member Author

Choose a reason for hiding this comment

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

clickCommandLog('click')
.then(() => {
expect(spyTableName).calledWith('Mouse Events')
expect(spyTableData).calledOnce
Copy link
Contributor

Choose a reason for hiding this comment

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

This is A LOT more readable 👍

*
* > Expander Column
* - When the log is a group log and it has children logs, it will display the chevron icon
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice comment

</div>
)
})
<div className='command-expander-column' onClick={() => model.toggleOpen()}>
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this command-expander-column element is eating clicks when the command isn't actually toggle-able/has no children. We could selectively add the onClick based on child presence as well?

Copy link
Contributor

@tbiethman tbiethman left a comment

Choose a reason for hiding this comment

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

Just had the one comment related to the expander column. I ran through the stated requirements in the PR description, everything seems to be working as expected. It's looking great!

@ryanthemanuel
Copy link
Collaborator

Everything seems to be working well. Nice work!

@emilyrohrbough emilyrohrbough merged commit 1232b07 into 10.0-release Apr 27, 2022
@emilyrohrbough emilyrohrbough deleted the new-cmd-log-styles branch April 27, 2022 15:23
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jun 1, 2022

Released in 10.0.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v10.0.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Jun 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants