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

Small refactor to separate signal layout building #211

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

Conversation

ziluvatar
Copy link
Contributor

@ziluvatar ziluvatar commented Feb 24, 2019

I'm working adding "Opt" support (and "alt" support after that). In my work in progress I found I will need to create the concept of "nested block" where signals inside "Opt" element will make it grow accordingly.

In order to progress towards that I had to refactor the signal layout processing to make it run once for the whole diagram and another per each nested block (which holds its signals).

I wanted to create this PR to separate that refactor from the change so it is easier to follow the upcoming changes. If I detect other refactor/changes that I can separate to have smaller PRs then I'll do it (unless in case @bramp you'd rather like the whole change at once).

I could have refactored it further or cleaner, I guess we can decide that once "Opt" is supported and before starting with the "Alt" one.

src/theme.js Outdated Show resolved Hide resolved
@ziluvatar ziluvatar force-pushed the refactor-for-future-nested-blocks branch from d903768 to 4ed67de Compare February 24, 2019 22:08
@ziluvatar ziluvatar force-pushed the refactor-for-future-nested-blocks branch from 4ed67de to 59b0438 Compare February 24, 2019 22:13
@alexrjs
Copy link

alexrjs commented Apr 4, 2019

@ziluvatar ... Cool sh*t... Tried out the gh_pages branch... Your refactoring worked fine for me... The bloated bower components are a little downer, but guess thats just a little cleanup in the Makefile to make it slimmer... Great work, anyhow... Thanks for sharing... Hope you make opt and alt work, cause that would be a nice addition... Since @bramp seems to have abandoned the support... Sorry, can't help with coding, just a user of this awesome tool... ;-)
/A

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