-
Notifications
You must be signed in to change notification settings - Fork 19
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
1791 Mugglify CIF Importer #1799
Conversation
7e4aca5
to
290730f
Compare
I see this is not ready for review yet, but just wanted to chime in and say I like the new look of things! A single page dialog is definitely the way to go (I got very fed up of the wizard when I was working on this stuff before!). |
Yeah, I think my view on wizards has changed to the polar opposite now - can't stand em! Particularly in the case of the CIF import, it felt like didn't give the users or the developers the best view on things. |
290730f
to
f81da8e
Compare
f81da8e
to
8bd65bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, minor comments.
Co-authored-by: Noella Spitz <[email protected]>
Co-authored-by: Noella Spitz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Co-authored-by: Noella Spitz <[email protected]>
This PR performs a significant refactoring on the
CIFImportDialog
, turning it from a wizard into a fully-featured single-page dialog. Aside from, IMHO, a better UX, a significant secondary reason was the desire to streamline the underlying CIF generation and move away from doing the full, multi-stage generation every time a parameter was changed.The dialog itself now looks like this, with a toolbox on the left-hand side containing all the tweakable parameters, but the majority of space given over to actually seeing what the generated structure will be:
In other news, we also remove the local
CoreData
object and just go for local instances of the necessary (temporary) objects. A few bug fixes also along the way. Lovely.Closes #1791.