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

Refactor DOM creation #81

Merged
merged 1 commit into from
Jan 15, 2018
Merged

Conversation

pluehne
Copy link
Contributor

@pluehne pluehne commented Jan 15, 2018

This refactoring simplifies the HTML necessary to build charts by moving the creation of required wrappers to the JavaScript side.

This approach has multiple benefits.

  1. First, the HTML part is more lightweight. Hence, future additions to the charts (such as action bars) will require much less changes to the HTML.
  2. Second, we have more control over the layout on the JavaScript side without having to touch the HTML part.
  3. Third, information concerning the chart content is grouped and separated from the layout part more nicely in this way.
  4. Finally, with this preparation, we could render placeholders more nicely in the future, as they have a distinct style now.

The existing charts are adjusted to these changes and produce pixel-wise identical visualizations. Additionally, this unifies the way how charts, tables, and collaboration graphs are rendered.

This refactoring simplifies the HTML necessary to build charts by moving
the creation of required wrappers to the JavaScript side.

This approach has multiple benefits. First, the HTML part is more
lightweight. Hence, future additions to the charts (such as action
bars) will require much less changes to the HTML. Second, we have more
control over the layout on the JavaScript side without having to touch
the HTML part. Third, information concerning the chart content is
grouped and separated from the layout part more nicely in this way.
Finally, with this preparation, we could render placeholders more nicely
in the future, as they have a distinct style now.

The existing charts are adjusted to these changes and produce pixel-wise
identical visualizations. Additionally, this unifies the way how charts,
tables, and collaboration graphs are rendered.
@pluehne pluehne force-pushed the patrick/refactor-dom-layout branch from fada710 to ab81166 Compare January 15, 2018 16:53
@larsxschneider larsxschneider merged commit 7d8d3d5 into master Jan 15, 2018
@larsxschneider larsxschneider deleted the patrick/refactor-dom-layout branch January 15, 2018 17:06
@filmaj
Copy link
Contributor

filmaj commented Jan 15, 2018

Would be good to update docs/README.md's Charts section to reflect these changes.

@pluehne
Copy link
Contributor Author

pluehne commented Jan 15, 2018

@filmaj: Good catch, I’ll do this later!

@larsxschneider
Copy link
Collaborator

@filmaj Thanks for the review 👍 Do you generally agree with these changes? It's easier to review with this link: https://github.com/Autodesk/hubble/pull/81/files?w=1

Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Yeah, looks great 👍

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.

3 participants