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

Adding another example from HOK workflow #30

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

albertotono
Copy link

@albertotono albertotono commented May 23, 2019

Thanks a lot to Radu for the great tool,
here there is another example. I used for the Revision Sheets.
There is the Airtable Issue of the 100lines to keep in mind.

Declarations

Check these if you believe they are true

Codebase

  • The code base is in a better state after this PR
  • Is documented according to the standards and ready for inclusion with release notes.
  • User facing strings, if any, are extracted into *.resx files

Testing

  • The level of testing this PR includes is appropriate
  • The solution has been compiled, loaded and tested in current Dynamo version (1.3)
  • All tests pass using the self-service CI (if present).

Changes

  • Snapshot of UI changes, if any.
  • Version change follows Semantic Versioning and is updated in the assembly DLL version.

Thanks a lot to Radu for the great tool,
here there is another example. I used for the Revision Sheets.
There is the Airtable Issue of the 100lines to keep in mind.
@radumg
Copy link
Owner

radumg commented Jul 8, 2019

Many thanks Alberto for the sample, will help lots of people using Airtable. I'll have a look in Dynamo and let you know if I have any questions!

@radumg radumg added this to the OSS + Docs milestone Jul 8, 2019
@radumg radumg self-assigned this Jul 8, 2019
@radumg radumg self-requested a review July 8, 2019 11:09
@radumg
Copy link
Owner

radumg commented Jul 22, 2019

Thanks again @albertotono for the PR. I'd like to merge this, but with a few changes. If you're happy to make them, I've left notes & tasks for you below. Let me know if you have any questions!

Dependencies

I'd like all samples for DynaWeb to require nothing more than Dynamo + DynaWeb to make it as accessible as possible.

Other packages

  • remove any dependencies on other packages

The samples depends on JsonData and archi-lab packages. The JsonData dependency in particular is not needed, since you should be able to perform the same operations with the nodes in DynaWeb Utils. Nothing against these awesome 2 packages from my friends, but samples for this repo should be dependency-free as much as possible.

If you have questions on how to achieve the same thing as you're currently doing with JsonData, let me know. The current sample is basically parsing the JSON 3-4 times at least in the graph, which is redundant.

Revit

image

  • remove dependency on Revit

Dynamo : open-source & free.
DynaWeb : open-source & free.
Revit : closed-source & needs license

So, I'd prefer it if we kept this sample Revit-free. Doesn't mean we can't have Revit samples ever, but prefer to keep samples focused on a single thing at a time , in this case that is interaction with Airtable. I would suggest to use a string/code block that has a list of sheet names and add a Note to replace this with a list from Revit that users can handle themselves.

Documentation

There's several notes that refer to sheets, I suspects it's because this graph started from the old Google Sheets sample from BiLT EUR dataset. Would you be able to please

  • replace mentions of sheets & update docs to match Airtable concepts & terminology

For reference, here's the terminology Airtable uses :

image

  • cleanup any rogue documentation notes

image

Copy link
Owner

@radumg radumg left a comment

Choose a reason for hiding this comment

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

Please make changes as highlighted in the tasks above.

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.

2 participants