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

List Navigation Reorder #19

Closed
jrbostic opened this issue Sep 4, 2017 · 24 comments
Closed

List Navigation Reorder #19

jrbostic opened this issue Sep 4, 2017 · 24 comments

Comments

@jrbostic
Copy link
Collaborator

jrbostic commented Sep 4, 2017

Allowing for reorder of scenes from list navigation window. I'm willing to look into it myself, but I haven't done any Atom package work before... So, looking for a complexity rating and feasibility assessment.

@morekitsch
Copy link
Collaborator

morekitsch commented Sep 4, 2017 via email

@jrbostic
Copy link
Collaborator Author

jrbostic commented Sep 4, 2017

Okay. Well, I'm looking at your original pull request code now to get familiar with that piece. I probably have a ways to go, just because I'm new to both atom package dev and coffeescript. But, we'll see. It can't be too incredibly difficult. The overview list items need to be drag/drop, determine where it was (beginning to end), remove and add contents at new index. I'm sure it won't be that easy... but... sounds good when I say it like that... :)

@jrbostic
Copy link
Collaborator Author

jrbostic commented Sep 5, 2017

So far, so good. Using sortablejs node module to reorder items. Looks like the next step is writing out a new file and replacing old when item is moved. At least, that's how I'll do it to start off with. I'd like something more efficient, but I just want to get the basics working first. I may need some help wrapping my head around the way the project is managing dependencies/imports though. I've got those coming in fine... but I'm sure there's something I'm missing for the final draft. Will include you in a code review once it works.

@jrbostic
Copy link
Collaborator Author

jrbostic commented Sep 5, 2017

Alright. Got it working! Will clean up and submit for review soon.

@jrbostic
Copy link
Collaborator Author

jrbostic commented Sep 5, 2017

See Pull Request #20 ... still some cleanup needed, but that's the general picture.

@jrbostic
Copy link
Collaborator Author

jrbostic commented Sep 7, 2017

So, I've done my best to build off of the work that you had done there, morekitsch. I also added some unit tests.

@jrbostic
Copy link
Collaborator Author

jrbostic commented Sep 8, 2017

@morekitsch About the clip on the README.md? I just noticed it. Is that how outline view is supposed to look? Making me nervous... cause all I've seen is a straight up list of items.

@morekitsch
Copy link
Collaborator

morekitsch commented Sep 8, 2017 via email

@jrbostic
Copy link
Collaborator Author

jrbostic commented Sep 8, 2017

Well, thanks for the complimentary tone. :) But apparently (after reviewing)... I work faster than I think. Looks like I skipped over some important details here. I was working with a flatter structure, not realizing the support for nested elements... even though I noted it in the code. Could be an easy update, but I'm not counting on it. There's probably some tough questions to answer among the rubble. Then there's tests... Anyway, do delay manual testing on this. I'll need more time. On the plus, side, I'll push forward with my pdf export pull request sooner...

@jrbostic
Copy link
Collaborator Author

jrbostic commented Sep 8, 2017

@morekitsch So, I've spent a little time mulling over the challenge of doing this thing all the way through... and I just wanted to jot it all down... for reference, clarity, feedback, posterity... whatever it amounts to.

Yikes:
Yea, so, I've been looking over and analyzing what it would take to do this... and do it well... and I'm a little daunted by the task. I've come across a couple available solutions with some of the necessary functionality, but they're all pretty shady. People are trying to do it and struggling... or there just aren't the right configurations for this use.

Details:
The dep I was using, sortablejs, supports flat list structures... but even if I used it recursively, it wouldn't allow for dragging scenes across acts and whatnot because it relies on a singular list model for each instance. What we would actually need for this is to enforce some hierarchical relationships strictly, but also keep it loose in other tiers (so acts couldn't be dropped into scenes but scenes could be moved between acts and such). This sounds really hard. Especially on the testing side.

Consideration:
But maybe the problem here is preconceptions about the real problem.

Up until now I was framing the problem as a hierarchical issue because of the way formatting is (quite logically) applied to the html. But... maybe it doesn't need to be that way... maybe we can flatten the display hierarchy... give all the current fountain elements their own list item (with line numbers and all of that) inside of a single HTML list... and then apply CSS classes to mirror/approach the current appearance. Then blocks would be defined simply by their start and end lines, so they could all be managed individually. And sortablejs can do that.

However, this would create a problem for moving parents in the visual hierarchy. They would not migrate their children naturally with sortablejs. But we could then select all lines between the dragged parent and its next sibling element... and move them simultaneously... again, based on CSS.

Can you think of any reason why that wouldn't work?

@morekitsch
Copy link
Collaborator

morekitsch commented Sep 8, 2017 via email

@jrbostic
Copy link
Collaborator Author

jrbostic commented Sep 8, 2017

@morekitsch Good news! Looks like you may get to give it a try this weekend after all.

I used the approach described above and it was successful. A few minor tweaks to formatting, adding a dynamic class for depth, and a custom flattening of the scene data structure. Looks nearly identical to my eye.

The only caveat is that I haven't yet implemented the piece to drag "children" automatically with their visual "parents".

Since what I've done so far is more stable and better tested... and dragging children adds a number of new behavioral questions... I'm hoping to delay that work for a future branch as an additional feature. Another reason for this is the testing itself. I'd like more time to think about (1) what the desired behavior is and (2) how to effectively test it... before doing significantly more work. My personal view, at this point, is that it makes sense to split that into another unit of work.

My hope is that I'd be able to get this branch in, then get the pdf export branch in (which just needs a few tests now), and then return to the outline view issue. @

@morekitsch
Copy link
Collaborator

I was just able to check your work. It' super cool. I like the lock, though if I had a choice i'd color it so it's clear that it's a toggle of sorts. I didn't realize I thought it was part of the title for a minute when I first opened it up. Of course updated docks could help this. This is all very cool.

I did find a bug however. When I click on a section that is off screen (needs to be scrolled too) the app wont scroll unless I click in the edit-pane. Then since I clicked there I lose my focus so the line is no longer highlighted. As long as there is no scrolling involved things work normal. I think you must have set thing to not scroll unless the editor is active. That is fine as long as clicking to make the editor active doesn't lose my highlighted section.

@morekitsch
Copy link
Collaborator

Ya know, changing the icon color probably wont make it look more like a button. We can just make a better readme.

@jrbostic
Copy link
Collaborator Author

jrbostic commented Sep 29, 2017

Hey thanks. Wouldn't have been possible without your work, and team support.

I actually experimented quite a bit with different icon arrangements... my favorite from the octicons set was the original red "X" overlaying the lock. But... that was problematic. I did make an attempt to contact the octicon people and query about an "unlock" icon, although I'd bet that the issue will just be closed. If all else fails, I was hoping to look at font awesome or something that has icons that speak for themselves. To be continued...

As for the scrolling bug... I'm not seeing that behavior. Are there any console errors being thrown or anything like that? I actually made no changes to the logic there... but definitely let me know if there is something I can do to reproduce it? @superlou have you seen this?

@morekitsch
Copy link
Collaborator

morekitsch commented Sep 29, 2017 via email

@morekitsch
Copy link
Collaborator

morekitsch commented Sep 29, 2017 via email

@jrbostic
Copy link
Collaborator Author

Well, I think @superlou was on the track to update readme and release after PDF creation was in. Of course, I don't mind either way as long as docs are in sync with released version. This dilemma actually had me wondering about the pros/cons of having a develop branch.

Regarding the icon... Since the lock is an svg it would probably be quick work to modify the original into an "unlock". The only potential issue I foresee is width. It could mean that two new graphics would be needed if we wanted to avoid a jarring visual jump on transition. Or maybe css could be applied to give the appearance of equal width. It's an odd situation because the key and lock are logically complimentary, but not as intuitive as a lock and unlock combo.

In response to the personal note... I guess I'm more of a dabbler. What I've been using fountain for isn't even strictly film related. Embarrassingly, I've been working on a dystopian graphic novel script and preferred the more structured approach. But I'm also FT software engineer and married with 3 young kids, so a lot of my extra creative energy/time has been getting funneled into the coding work.

What kind of projects are you interested in? (Also, I used to live on the same block as legendary donuts, so I hear ya!)

@superlou
Copy link
Owner

I'm fine with updating the readme sooner rather than later. It was more that I was planning to release the next minor version after the PDF stuff is in, so it would need to be updated at that point.

@jrbostic
Copy link
Collaborator Author

Oh, right, because tagged versions still carry appropriate docs for that point in time. I was worried that people would see the readme and wonder why the feature wasn't present on their newest version. Got it though... My misunderstanding.

@jrbostic
Copy link
Collaborator Author

jrbostic commented Oct 3, 2017

@morekitsch Just wanted to let you know that I fixed the panel header for the outline view (while I was integrating PDF button), and in the process I moved back to the red X overlay for "unlocked" indicator. An open lock would probably still be better... but I didn't want it to come as a surprise if you did end up doing work on that.

@morekitsch
Copy link
Collaborator

morekitsch commented Oct 3, 2017 via email

@jrbostic
Copy link
Collaborator Author

jrbostic commented Oct 3, 2017

Yea, one big dream of mine (with the Afterwriting piece) is to add support for end users to configure output formatting... #27. That would add some versatility for sure... plus its kind of a freebie from the tool.

@superlou
Copy link
Owner

superlou commented Oct 6, 2017

Closing with the merge of the basic functionality. Thanks!

@superlou superlou closed this as completed Oct 6, 2017
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

No branches or pull requests

3 participants