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

Dyn 4843 watch node smaller #12855

Merged
merged 28 commits into from
May 4, 2022
Merged

Dyn 4843 watch node smaller #12855

merged 28 commits into from
May 4, 2022

Conversation

jesusalvino
Copy link
Contributor

@jesusalvino jesusalvino commented May 4, 2022

Purpose

Fixing the UI Tests based on the new WatchNode styling

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

@QilongTang

FYIs

@RobertGlobant20 @filipeotero

@jesusalvino
Copy link
Contributor Author

imagen

@pinzart90 pinzart90 requested review from pinzart90 and removed request for pinzart May 4, 2022 16:59
@QilongTang
Copy link
Contributor

@jesusalvino What is the real change here? Is it just 64608e0? If this is the case, next time please use a clean branch

{
if (_vm.Children != null)
{
if (!_vm.Children[0].IsCollection)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can there be a case where _vm.Children.Count == 0 at this point ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so since I have tested with only a single line and multiple lines, for both cases the node has at least one element

{
if (!_vm.Children[0].IsCollection)
{
double requiredWidth = (_vm.Children[0].NodeLabel.Length * 7.5);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 7.5 ?
A comment at least

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the width factor for each character, it has been approved / and evidenced here #12829, this current PR is focusing in the tests.

@jesusalvino
Copy link
Contributor Author

@jesusalvino What is the real change here? Is it just 64608e0? If this is the case, next time please use a clean branch

yes only the tests / for sure sorry for the confusion

@QilongTang
Copy link
Contributor

@jesusalvino What is the real change here? Is it just 64608e0? If this is the case, next time please use a clean branch

yes only the tests / for sure sorry for the confusion

Can you add a comment just in case? Then let's wait for PR checks to finish. Thanks for the fix!

@QilongTang
Copy link
Contributor

@jesusalvino Please also resolve the conflict, this is also a side effect of not using clean branch

@jesusalvino
Copy link
Contributor Author

@jesusalvino Please also resolve the conflict, this is also a side effect of not using clean branch

Done

@QilongTang QilongTang merged commit 0731b85 into DynamoDS:master May 4, 2022
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.

3 participants