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

DYN-2353 AnyTrue and AnyFalse Nodes #11287

Merged
merged 6 commits into from
Dec 4, 2020
Merged

DYN-2353 AnyTrue and AnyFalse Nodes #11287

merged 6 commits into from
Dec 4, 2020

Conversation

StudioLE
Copy link
Contributor

@StudioLE StudioLE commented Nov 30, 2020 •
edited
Loading

Purpose

JIRA: DYN-2353

List.AnyTrue will return true if any sublist contains a true boolean.
List.AnyFalse will return true if any sublist contains a false boolean.

DYN-2352 AnyTrue and AnyFalse Nodes

image

image

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated

Reviewers

@mjkkirschner @QilongTang @mmisol

FYIs

@Amoursol

@mmisol mmisol self-requested a review November 30, 2020 21:03
mmisol
mmisol reviewed Nov 30, 2020
View reviewed changes
Copy link
Collaborator

@mmisol mmisol left a comment

Choose a reason for hiding this comment

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

Found some minor issues with code formatting but it looks good from a technical POV.

Regarding the icons, I'm not sure it's easy to realize which one is which. Copying @Amoursol to get his opinion.

test/DynamoCoreTests/Nodes/ListTests.cs Outdated Show resolved Hide resolved
test/DynamoCoreTests/Nodes/ListTests.cs Outdated Show resolved Hide resolved
@Amoursol
Copy link
Contributor

Amoursol commented Nov 30, 2020 •
edited
Loading

Found some minor issues with code formatting but it looks good from a technical POV.

Regarding the icons, I'm not sure it's easy to realize which one is which. Copying @Amoursol to get his opinion.

Hello @StudioLE - Can you please have a go at creating icons that reference similar iconography to the existing TrueForAll and TrueForAny nodes? Suggestion: For AnyTrue, 3x Red boxes, 1x blue box inside red box set, surrounded by white master box and question mark icon. Invert box colours for (3x Blue, 1x Red) for AnyFalse.

image

@StudioLE
Copy link
Contributor Author

StudioLE commented Dec 2, 2020

Found some minor issues with code formatting but it looks good from a technical POV.
Regarding the icons, I'm not sure it's easy to realize which one is which. Copying @Amoursol to get his opinion.

Hello @StudioLE - Can you please have a go at creating icons that reference similar iconography to the existing TrueForAll and TrueForAny nodes? Suggestion: For AnyTrue, 3x Red boxes, 1x blue box inside red box set, surrounded by white master box and question mark icon. Invert box colours for (3x Blue, 1x Red) for AnyFalse.

Hi Sol,

I've come up with a couple of options based on your description but if I'm off the mark could you provide a markup to show what you mean?

Option 2

AnyTrue
AnyTrue Large
AnyTrue Small

AnyFalse
AnyFalse Large
AnyFalse Small

Option 3

AnyTrue
AnyTrue Large
AnyTrue Small

AnyFalse
AnyFalse Large
AnyFalse Small

I should probably also add that the original option was designed for consistency with the existing icons AllTrue and AllFalse icons:

image

@Amoursol
Copy link
Contributor

Amoursol commented Dec 2, 2020

@StudioLE - Can we please go with a modified Option 3: Boxes inside boxes inside a square gets a little lost at 32px, so the small icon would be hard to see.

Jingyi, our UX designer has provided a mock-up of what would match the shape language of the other AllFalse / AllTrue icons, as below:

image

@QilongTang QilongTang merged commit 09c95ed into DynamoDS:master Dec 4, 2020
QilongTang pushed a commit that referenced this pull request Dec 4, 2020
@StudioLE @QilongTang
DYN-2353 AnyTrue and AnyFalse Nodes (#11287)
6a7e4b1 
* Added AnyTrue and AnyFalse nodes

* Added unit tests for List.AnyTrue and List.AnyFalse

* Added tests for List.AnyTrue and List.AnyFalse

* Added icons for AnyTrue and AnyFalse

* Added AnyTrue and AnyFalse icons to libraryitems.json

* Fixed formatting
@QilongTang QilongTang mentioned this pull request Dec 4, 2020
Merged
8 tasks
QilongTang added a commit that referenced this pull request Dec 4, 2020
@QilongTang @StudioLE
DYN-2353 AnyTrue and AnyFalse Nodes (#11287) (#11329)
e134093 
* Added AnyTrue and AnyFalse nodes

* Added unit tests for List.AnyTrue and List.AnyFalse

* Added tests for List.AnyTrue and List.AnyFalse

* Added icons for AnyTrue and AnyFalse

* Added AnyTrue and AnyFalse icons to libraryitems.json

* Fixed formatting

Co-authored-by: Laurence Elsdon <[email protected]>
@StudioLE StudioLE mentioned this pull request Dec 14, 2020
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmisol mmisol mmisol left review comments

@QilongTang QilongTang QilongTang approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@StudioLE @Amoursol @QilongTang @mmisol