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

E movement #1623

Merged
merged 6 commits into from
Aug 18, 2021
Merged

E movement #1623

merged 6 commits into from
Aug 18, 2021

Conversation

pciavald
Copy link
Contributor

@pciavald pciavald commented Mar 28, 2021

This is how we implement it for us. I needed to refactor to flexbox (#1622) for easier enabling/disabling of the component. Feel free to just close if you don't want it in upstream, or use only the flexbox refactor.

  • implement new configuration showExtruderControl defaulting to false
  • control extrusion in control component if showExtruderControl set to true in user config

image

Fixes #1591
Closes #1883

@pciavald
Copy link
Contributor Author

pciavald commented Mar 28, 2021

It all works, but i still see this error clearly related to my usage of the configService, i guess i missed something to load the service properly ?

image

@UnchartedBull
Copy link
Owner

Great idea to make this configurable! Then there is nothing in the way of merging this into main. Just two little things:

The config error should only show up if the devTools reload your app. The problem here is that Angular somehow skips the initialisation process if a different view than mainView is open. This shouldn't happen on the Pi, since the UI is not being reloaded there. So nothing to worry about :)

@pciavald
Copy link
Contributor Author

Thanks, I'll check that! Glad you like the idea of making it configurable!

About the new config i was thinking to let it optional and not include it in the config, only setting the default value to false. This way when a config does not have the property, it does not get in the way, otherwise it can be set to false (no effect) or true. Do you prefer adding them to false anyway ?

@pciavald pciavald marked this pull request as draft March 29, 2021 16:14
@UnchartedBull
Copy link
Owner

Oh I see how this is working now. That's definitely a creative way to exploit the lazy type checking of JS :D.

Would be great if this could be a required part of the config, just to make everything a bit more consistent (should only be two lines of code to add this attribute during upgrades). Thanks! :)

@pciavald
Copy link
Contributor Author

will do!

@pciavald
Copy link
Contributor Author

pciavald commented Apr 6, 2021

I've just noticed that this may break themes.

@pciavald pciavald added this to the v3.1.0 milestone May 3, 2021
@stale
Copy link

stale bot commented Jun 2, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale issue label Jun 2, 2021
@pciavald pciavald removed the stale Stale issue label Jun 2, 2021
@pciavald
Copy link
Contributor Author

pciavald commented Jun 2, 2021

note to self: pushed this branch to upstream in order to rebase #1895 on it

@stale
Copy link

stale bot commented Jun 22, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale issue label Jun 22, 2021
@UnchartedBull UnchartedBull added enhancement New feature or request and removed stale Stale issue labels Jun 23, 2021
@UnchartedBull UnchartedBull modified the milestones: v3.1.0, v2.3.0 Jul 28, 2021
@UnchartedBull UnchartedBull marked this pull request as ready for review August 18, 2021 10:33
@UnchartedBull UnchartedBull merged commit d488844 into UnchartedBull:main Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Controll manual extrusion - "feature request" Add control for E axis
2 participants