Skip to content

LayoutSpecs Comparison #8189

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
Merged
QilongTang merged 3 commits into from
Sep 20, 2017
Merged

LayoutSpecs Comparison #8189

QilongTang merged 3 commits into from
Sep 20, 2017

Conversation

@QilongTang
Copy link
Contributor

@QilongTang QilongTang commented Sep 19, 2017 •
edited
Loading

Please Note:

  1. Before submitting the PR, please review How to Contribute to Dynamo
  2. Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
  3. PRs will be reviewed from oldest to newest
  4. If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
  5. PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
  6. PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
  7. PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

I finished the comparison between the layoutSpecs.json in Librarie.js repo and the one in Dynamo Repo. And found a few differences:

  1. There is a Keys sub category in Librarie.js which is not there in Dynamo
    image
    It turned out Dynamo is correct.

  2. There is a Label sub category which got deleted in Librarie.js but still there in Dynamo. What should we do there? That sub-category is hidden in this PR

  3. All resource icons and node paths are different in both json which makes it pretty hard to maintain both. What is the reasons behind it? @sharadkjaiswal

  4. A few icons for category is removed but I assume for good purpose. E.g. Geometry->Modifiers->Geometry

  5. We really need to stop syncing this json everywhere and create guidlines for whichever client willing to add new nodes. Hope this PR could be the discussion point of a new baseline.

Declarations

Check these if you believe they are true

  • The code base 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.

Reviewers

@mjkkirschner @ramramps

FYIs

@Racel

@QilongTang QilongTang added DNM Do not merge. For PRs. WIP labels Sep 19, 2017
@mjkkirschner
Copy link
Member

mjkkirschner commented Sep 19, 2017

2 . Label should be hidden (pretty sure, not 100%)

@sharadkjaiswal
Copy link
Contributor

sharadkjaiswal commented Sep 19, 2017

library.js layout spec should be seen as a prototype so no need to keep them in sync.
I am not sure if we want Keys as a separate category, it somehow breaks the visual flow. @Racel

@Racel
Copy link
Contributor

Racel commented Sep 19, 2017 •
edited
Loading

@QilongTang - I don't think you have the proper layout spec. We already fixed the keys issue. Something fishy is going on here. I am not seeing this problem in the 2.0 build I have. I would suggest reverting the code.

image

Also, not seeing the label sub-category

@ramramps
Copy link
Collaborator

ramramps commented Sep 19, 2017

@QilongTang Is this required for Dynamo 2.0? I don't trust librarie.js LayoutSpec. We need to do the comparison carefully, and I believe Dynamo /Revit layoutspec's are modified based on what they support.

@QilongTang
Copy link
Contributor Author

QilongTang commented Sep 19, 2017 •
edited
Loading

@Racel The label sub-cate is there in Dynamo master build
image
image

@ramramps @Racel I've done the comparison very carefully :) This is not meant for submission but a discussion thread so that we know what is the state of current Dynamo's LayoutSpecs. I will close this PR since Keys should not be there, but I want to just confirm the story around Label.

@ramramps
Copy link
Collaborator

ramramps commented Sep 19, 2017

@QilongTang thanks..can you open an issue instead?

@QilongTang QilongTang added PTAL Please Take A Look 👀 and removed DNM Do not merge. For PRs. WIP labels Sep 19, 2017
@QilongTang
Copy link
Contributor Author

QilongTang commented Sep 19, 2017

@mjkkirschner @ramramps I have confirmed that this sub-category should be hidden so let's deal with it in this PR since it's such a small change.

@mjkkirschner
Copy link
Member

mjkkirschner commented Sep 19, 2017

LGTM

@QilongTang
Copy link
Contributor Author

QilongTang commented Sep 20, 2017

@mjkkirschner @ramramps @Racel Merging this so we can hide this node for daily build users. We should discuss handling layoutSpecs updating while fixing nodes from audit result, but for now I think we all agree that the one in Librarie.js repo does not need to be maintained in the future.

@QilongTang QilongTang merged commit 3faad2b into master Sep 20, 2017
@QilongTang QilongTang deleted the LayoutSpecMapping branch September 20, 2017 05:06
@QilongTang QilongTang removed the PTAL Please Take A Look 👀 label Sep 20, 2017
mjkkirschner pushed a commit that referenced this pull request Aug 18, 2020
@RobertGlobant20
Dyn 3026 code coverage analysis folder (#10989)
8ee9b0e 
* DYN-3026-CodeCoverage-AnalysisFolder

I created several test methods for increasing the code coverage in the next classes (Analysis project).
- Label
- SurfaceData
- AnalysisExtensions
- Utils

Also I removed two methods from the SurfaceData.cs file since both have cero references and both are private.

* DYN-3026-CodeCoverage-AnalysisFolder

Fix for make the Analysis assembly visible to the AnalysisTests project.

* DYN-3026-CodeCoverage-AnalysisFolder-CodeReview

Changes asked by Aparajit.

* DYN-3026-CodeCoverage-AnalysisFolder-CodeReview2

I re-wrote the test for the Label class since Michael asked me to use a Label inside a Code Block node, then I created 3 dyn files with one CodeBlock creating a Label, one compiling and other two with errors (on purpose) when passing parameters to the Label.ByPointAndString method.

Also by Michael response I undid the changes done by Aaron in the PR #8189.

Finally I applied some changes that the developer Ian Keough did like 5 years ago (HelixRenderPackage.cs -> CleanTag) but were discarded in the PR below, with the code as it is now in the Dynamo repo  when creating a Label inside a Code Block it shows a strange symbol in the Background Preview. The only way to show the label text correctly was using the syntax "label:text", then Ian's discarded change was re-applied by me and now we don't need to use the weird Label syntax.
#4637

* DYN-3026-CodeCoverage-AnalysisFolder-CodeReview2

I applied some changes that the developer Ian Keough did like 5 years ago (HelixRenderPackage.cs -> CleanTag) but were discarded in the PR below, with the code as it is now in the Dynamo repo when creating a Label inside a Code Block it shows a strange symbol in the Background Preview. The only way to show the label text correctly was using the syntax "label:text", then Ian's discarded change was re-applied by me and now we don't need to use the weird Label syntax.
#4637

* DYN-3026-CodeCoverage-AnalysisFolder-CodeReview2

I moved the LabelTests to the DynamoCoreWpfTests project inside the Libraries folder.
I removed several dll in the AnalysisTests project that are producing erros in the mono.sln.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@QilongTang @mjkkirschner @sharadkjaiswal @Racel @ramramps