Fix for #206, show authors, issues & articles editors in Dialog boxes on admin site #217

tongxin97 · 2017-09-25T21:56:40Z

EDIT Oct 1, 17:

Moved away from the scrolling fix and uses Material UI's Dialog to show the editor components.

Automatically scroll down to the Article Editor/Author Profile area when selecting certain one article/author.
The scroll/jump effect isn't very pretty but it does the trick.

See this discussion on Github for more info on the fix: remix-run/react-router#394

EDIT BY EMIL: resolves #206 (just so the issue is automatically closed when we merge this PR)

emilgoldsmith

Good first attempt, and I think the general code is pretty correct and good. A few things though:

Since you're calling it in componentDidUpdate this will be called every single time the component changes anything, that could cause all kinds of weird edge cases, especially as the hash doesn't change as you scroll to different places. One edge case I can imagine is that we later also made a hash for the top of that page to go "back to top" or something, if that was your url and you went down and typed something in the article controller as soon as you typed something your screen would instantly jump up to the top again.
While you could correct 1. to make it work every place, instead of duplicating code everywhere (the first method would be just modularizing this to a function and put it in lib/utilities.js but there's a better method) why don't you do as described in the comment you linked me to? this one. I really liked that, using the Router's onUpdate prop, you can find the router in src/client-scripts/admin-client.js
Other than that stuff looks good, the ids and the links all look good. So if you just move the scroll code to the onUpdate Router prop as I said above it should be ready to merge. By then I'll also look at the scroll animation and maybe we can use the react-scroll library or something if it's too ugly? Also, I think you could add this to the IssueController too.

emilgoldsmith · 2017-09-25T23:43:00Z

src/components/editor/EditorArticleController.jsx

@@ -119,6 +119,17 @@ export default class EditorArticleController extends FalcorController {
      // The update wasn't due to a change in article
      this.debouncedHandleFormStateChanges();
    }
+    // scroll to article editor once page loads
+    const { hash } = window.location;


Nice, you have good knowledge of ES6 :)

I agree with the comments. Didn't find the Router code at first glance, thanks for pointing me to it! Will update later tonight or tomorrow.

Sounds good, just write a comment on the PR when you're ready for another review

Or even better, if we're trying to enforce the Zenhub workflow I just did what I should've done and moved the issue and PR back to in progress, and when you're ready for review just move them back into Review/QA

tongxin97 · 2017-09-26T03:58:38Z

One thing that's happening with the Issues page is, since the Articles and Categories components take a second to load, the scroll doesn't fully take effect when you click on those buttons as soon as the page loads. It functions normally only after those components are rendered. Actually, same thing happens with the other pages but to a less extent, so I was able to fix it for them with requestAnimationFrame and setting a timeout to try scrolling again if the component hasn't rendered at the beginning, as described in this comment.

tongxin97 · 2017-09-26T04:02:20Z

I can also explore react-scroll and see if it can add a smooth scroll animation.

zanemountcastle · 2017-09-26T16:24:03Z

I think what would be really elegant would be to simply style the article/author editor components to show up as modals. In this way they would be actually overall content (with the other content darkened) and then when they are dismissed, they disappear.

This could be achieved pretty simply with a dialog tag from the Material UI library we use. This would mean we don't have to add any more extraneous dependencies and we won't even have to worry about refactoring things. Check it out here: http://www.material-ui.com/#/components/dialog

emilgoldsmith · 2017-09-26T16:28:24Z

Hmm yeah, that's a solid idea, shall we settle on that instead of the scroll function? It may also be more mobile friendly even though as I said it's not our main priority. And I guess it would never be the case that you somehow wanted to look at both the picker and the article at the same time so a modal could be a good fix

tongxin97 · 2017-09-26T16:36:33Z

Okay I'll explore Material UI and see what I can do! Thanks @zanemountcastle and @emilgoldsmith for the comments. I'll let you know how it goes. And this way the rendering glitch will go away too since there's no scrolling anymore.

emilgoldsmith · 2017-09-26T18:23:30Z

Sounds good! Thanks for the flexibility even though we completely changed direction :).

tongxin97 · 2017-09-27T20:20:00Z

Just tried Dialog on the articles page.

One linter error:
error Value must be omitted for boolean attributes (react/jsx-boolean-value)
I'm not sure what's wrong with the boolean value.. it'd be great if you guys can check it out.

Other than that, I think this Material UI solution looks pretty nice. And I'll add it to the other pages if you guys think it's in good shape. @zanemountcastle @emilgoldsmith

tongxin97 · 2017-09-27T20:21:50Z

src/components/editor/EditorArticleController.jsx

          >
-            <SelectField


Only changed the indentation below this line. not sure why Git created such a mess with the blocks.

Yeah I'm afraid git isn't always so smart <.<. It seems like it's taking line 363 as a constant because it used to be an end of React element of Form and now it's an end of React element Dialog, but git thinks that's a line that you "kept"

emilgoldsmith · 2017-09-27T20:25:18Z

Thanks for the swift development cycle @CynthiaTong I'm really happy with you work until now! Just to quickly address your linting question, it's probably giving the error on the selectfield where you did autoWidth={false} since if you don't specify it at all it'll automatically be set to false (or undefined to be more specific which is falsy). For the true case if you just specify a prop such as autoWidth instead of autoWidth={true} they're actually equivalent.

If you personally feel like it's important for readability and transparency of the SelectField API I could be okay with you adding a // eslint-disable-line react/jsx-boolean-value comment for the line, but I'll leave that up to you :).

I'll take a more thorough look this evening and get back to you with a proper review!

tongxin97 · 2017-09-27T20:37:39Z

@emilgoldsmith thanks :).
I didn't say very clearly before - the linter error actually occurs in these lines inside the Dialog block:

377: modal={false}
378: autoScrollBodyContent={true}

These are pretty standard attributes of Dialog, so I think it should be fine to keep them the way they are.

emilgoldsmith · 2017-09-27T20:41:55Z

Oh @CynthiaTong in that case all the linter wants you to do is simply write autoScrollBodyContent instead of autoScrollBodyContent={true} and that'll fix it :)

tongxin97 · 2017-09-27T20:53:13Z

Amazing quick fix. Let me know what you think about the rest of the code. Thanks!

emilgoldsmith

Generally good work again, I really like how the UI turns out (also just checked it out on mobile haha, really doesn't work as you said, but as we mentioned it's not a priority issue). You did break some functionality though and a few minor comments.

The problem is you removed the submit button from the form and just made it a close dialogue button. The easiest thing is to just return that. I think the choice you made of putting the save changes button as a fixed button in the bottom (or in this context an action button) looks pretty good, the other possibility is just putting it in the bottom where it was and not adding any "actions." I personally think both are pretty okay, but also think @CynthiaTong's choice looks quite good actually. @zanemountcastle any opinion on this as it's front-end stuff?

Hmm I have a few thoughts about this though... Also because I feel like maybe when we do this modal we don't even need the routing anymore? As in, maybe we just implement it outside of routes, and just make the dialog appear on click.

another small thing is I would probably make Dialog the parent of all the divs in this, aka the most outer html element.

I have to think a bit more about this, and maybe @zanemountcastle has some inputs as well, but actually in class so should go back to listening. @CynthiaTong also feel free to voice your opinion if you have thoughts about it, and don't worry changing anything until we're sure what exactly is the best approach.

emilgoldsmith · 2017-09-27T21:57:18Z

src/components/editor/EditorArticleController.jsx

@@ -348,93 +356,107 @@ export default class EditorArticleController extends FalcorController {
        }
      }

+      // Dialog action button
+      const actions = [


As I'll elaborate this button doesn't provide the functionality it previously did

Are you saying it doesn't save changes now? That's a big issue. I'll look at it more carefully.

Yeah, doesn't save changes anymore, this button used to submit the form by being inside the form and being of type="submit" but now it no longer does that.

It's okay if you wait a bit with implementing changes though, at least until we just decide finally on what's the optimal way to implement this.
I just have quite a lot of stuff to get done tonight, but it's ruminating in the back of my head and I think I nearly have a good structure, but yeah, have a lot of stuff I need to finish tonight, sorry.

no it's totally cool. Good luck with whatever you're doing. Things can wait :).

emilgoldsmith · 2017-09-27T21:58:49Z

src/components/editor/EditorArticleController.jsx

+          disabled={!this.state.changed || this.state.saving}
+        />,
+      ];
+
      return (
        <div style={styles.innerPaper}>


Hmm don't you think this styled div should be within the Dialog maybe?

Yes indeed. I'll fix this.

tongxin97 · 2017-09-28T00:20:42Z

@emilgoldsmith as for the point you raised about routing, I don't have enough experience yet with react-router so I'm not 100% sure of the benefits of using routing vs. just using Dialog. If these two approaches are roughly equivalent (in terms of efficiency, code consistency, data flow, etc), just using Dialog could be simpler. Although one good thing about routing I can think of is, people can share the url to the specific article editor/issue/author profile if they want (correct me on this if I'm wrong, I'm assuming people can access the linked page directly after login, skipping the articles/issues/author selection step)

emilgoldsmith · 2017-09-28T13:17:14Z

Okay, it has ruminated!

So I definitely think we should keep routing, not only because of the point @CynthiaTong raises:

Although one good thing about routing I can think of is, people can share the url to the specific article editor/issue/author profile if they want

which would be even more relevant when we implement Google login which would probably entail cookies as well so you don't need to login all the time. And maybe sharing links with each other isn't the most common practice for the site, but maybe revisiting a link yourself could be. And the most important thing I can think of is actually that without routing the back button wouldn't work, and that would truly suck, so we definitely need the routing.

So given that is set in stone, I can now list all the changes we need to make.

As you know @CynthiaTong put the dialog box as the top level html element. Also, you probably want to remove the Paper the modal is encapsulated in, in the EditorArticleListController.jsx file, as that is redundant, and then you can probably also remove the div that has the innerPaper styles all together, I'll let you play with that and see what you think works best, you could also put a Paper inside the Dialog I guess if that looks good, lots of possibilities.
When the modal closes right now we stay on the route which is bad for several reasons as it doesn't unmount the component, it doesn't clearly signal that we're on a different page (if you pressed the refresh button the article editor would pop up again), the back button wouldn't work as intended etc. So what I'm thinking is actually, we just remove the open key in state and make it a constant, so in the Dialog element just make open={this.state.open} to open, and then in your handleDialogClose handler use browserHistory.push like you can see in clickSearchSuggestions of EditorArticleListController.jsx to return to the parent route, this will automatically remove the modal.
Of course fix the saving as described above, I would probably suggest not having closing the modal having anything to do with saving at all, I could imagine people doing intermediate saves and not wanting the modal to close, doesn't that make sense? So if you want, you could make both a save button and a separate close button in the actions, or just leave it so you click on the side to close, or put both buttons at the bottom instead of in the actions, many choices, I'll let you decide which one to go with. As mentioned earlier for submitting the form the easiest is of course to have it inside the form tag as then you can just make it type="submit" otherwise you'll probably need to do something like using a ref as explained here to the form tag, and then calling .submit() on it, which is a bit more bothersome but again I'll leave it up to you.
It's super important that you shouldn't be allowed to leave the modal while the form is saving, so you should add an if statement in the handleDialogClose that doesn't actually reroute if the form is currently saving.

Okay, that was my 200 cents ;). Have fun working! (and also feel free to ask questions / make comments if you disagree with something)

emilgoldsmith · 2017-09-28T14:05:25Z

Actually just thought of something. The whole ref thing is totally overkill as we save everything in state, so there's actually no need (if you choose to go the "action" way in terms of UI) to do all that complicated stuff. You could simply call handleSaveChanges as the onClick handler for the save button, and then remove the event param and the top call to event.preventDefault as it would be redundant if it's not while submitting the form.

tongxin97 · 2017-09-28T22:50:55Z

@emilgoldsmith thanks a lot for the very informative comment and advice! I will be very busy these days for various reasons, so it'll take longer for me to implement all this. But advice noted and I'll try my best to complete it asap. Also I'll let you know what I think of the questions you mentioned as I go.

emilgoldsmith · 2017-09-28T23:30:25Z

Sounds good, thanks for the heads up on the "delay" :). As long as you keep up good communication we can tolerate basically anything.

zanemountcastle · 2017-10-01T09:16:41Z

Any update on this @CynthiaTong?

tongxin97 · 2017-10-02T02:07:04Z

@zanemountcastle sick over the weekend. I'll work on it tmr! (Update: actually looking at it right now

tongxin97 · 2017-10-02T03:18:56Z

Basically followed @emilgoldsmith's suggestions and moved the save changes button back inside the Dialog. The default way to close the Dialog is by clicking outside of it or pressing the esc key, so I just left it this way. I felt adding a close button somewhere would be kind of awkward, since there are already two buttons for saving and unpublishing.
Speaking of which, the unpublish procedure takes a pretty long time, and I don't see any changes reflected after it's been done (the unpublish button goes live again when you reopen the page).

Let me know what you guys think (I hope it finally works this time :D.
Need to implement similar stuff for the other pages still (Sorry for postponing the fix.. I was pretty sick :(

emilgoldsmith

What a refreshingly small diff now that you no longer need to change the indentation of anything :).

Just a quick change regarding keeping open in state as it's no longer relevant, should be a 30 second fix.

I'll just do a manual test now, but as far as I can see you barely changed anything so it should only be front-end stuff that could have problems.

Also just remembered that we still have the Paper wrapped around this component in the EditorArticleListController component yeah? We probably want to remove that. It might not actually render when it doesn't have any children, but it's definitely better not having it there if it doesn't make sense anymore.

Regarding the publish button not changing, yeah I noticed that a little while back as well, I believe I made an issue that's in one of our 80 issues on our todo list ;). But thanks for pointing it out.

Again, thanks for the good work! Very happy to have brought you on board.

emilgoldsmith · 2017-10-02T14:00:24Z

src/components/editor/EditorArticleController.jsx

-          <h2>Article Editor: {article.title}</h2>
+        <Dialog
+          title="Article Editor"
+          open={this.state.open}


let's just change this to just open (the implicit way of writing open={true}) since we never change this, we "close" this by routing away.

oh I kinda overlooked this.. will change it now!

and thanks for bringing me onboard :D

emilgoldsmith · 2017-10-02T14:00:45Z

src/components/editor/EditorArticleController.jsx

@@ -36,6 +39,7 @@ export default class EditorArticleController extends FalcorController {
      image: updateFieldValue.bind(this, 'image', undefined),
    };
    this.safeSetState({
+      open: true,


Let's remove this as described below.

emilgoldsmith · 2017-10-02T14:12:11Z

Okay just did a real test, works really well, I like it a lot. And also just to reply to your last points of your comment, yeah I completely agree with keeping the escape button and clicking outside to escape, maybe if we were doing mobile stuff making a closing X in the corner could make sense but as we're not currently supporting mobile I don't think that's relevant, so good call in my opinion :).

And as for being sick, don't worry, you weren't really that slow.

So yeah, just quickly remove open from state and the Paper from EditorArticleListController and then feel free to implement similar things for Authors, and maybe when we're doing this modal thing it's not necessarily relevant for the Issues both as the top bit is small so the missing something changing UX problem shouldn't be relevant and it may look a bit weird with the tabs??

I'm not sure about the issues one, feel free to try it out and make a call whether you like it or not @CynthiaTong and present whatever you think works best to us.

emilgoldsmith · 2017-10-02T20:12:38Z

And yeah the issue I talked about was #184

tongxin97 · 2017-10-03T00:33:04Z

Alright! I'll see how Dialog goes with the Issues page @emilgoldsmith. But you're right about it not being such a UX problem on that page - since the main tab has a very short height people can see it alright without scrolling down or anything.
I'll push the changes tmr!

emilgoldsmith · 2017-10-03T00:38:12Z

Sounds good, I'll look forward to seeing the full PR

zanemountcastle · 2017-10-03T06:15:11Z

This looks really good @CynthiaTong! This is definitely much more user-friendly than having to scroll down to see the article editor. I also think that this same kind of thing will look great in the authors and issues sections. Keep up the good work! :)

tongxin97 · 2017-10-04T01:56:09Z

Authors page looks good.

There is an issue with the Issues page (haha puns). Tabs don't play well with Dialog - each tab's child component has their own height, and the Dialog box doesn't auto-adjust to that height change when you click on the tabs, the result is that contents don't show up in a pleasant manner. I'm not very familiar with this kind of "reloading" issue I guess, so if you guys have anything to say about it pls do!

I included the changes in EditorIssueListController.jsx in the latest commit just so you can see that glitch.

emilgoldsmith

This looks really good @CynthiaTong, I love it :D.

I especially like the idea of these modals (thanks @zanemountcastle !) as I feel like it opens up the easy possibility of doing things like allowing the editing of an article from the part where you're adding issues to an article (which would be super convenient) etc.

I just quickly pushed a small commit that just removes the styles.innerPaper style object as I realized that was no longer in use, it was much easier for me to just push than tell you to do it as it was such a small insignificant thing.

And yeah I see what you mean about the issue stuff, what do you think @zanemountcastle ? If it was up to me I think I'd just drop the modal for that one for now, and I also don't think it's an issue there as @CynthiaTong and I mentioned earlier since the top part is so short. Unless you have a nice fix, @zanemountcastle ?

(I'm only requesting changes because we need to figure out what to do with the Issue stuff, otherwise it looks great)

zanemountcastle · 2017-10-05T06:23:49Z

Okay, so my input on the issues modal thing. I think that we should just leave the issue editor as it was because:

the issue editor is quite a bit more complicated than the article or author editors and modals aren't really meant for that level of complexity and
The "Select an Issue" section is already small enough that it's immediately apparent to the users that the article shows up below when one is selected.

The author and article modals look great though and I know the editors will definitely appreciate them. 👍

It looks like this is ready to be merged into the site!

emilgoldsmith · 2017-10-05T12:05:58Z

Awesome, so we all agree!

So yeah @CynthiaTong if you just reset he issues stuff when you find the time we'll merge it in :)

emilgoldsmith · 2017-10-05T20:25:17Z

You forgot to remove the redundant Paper elements in the parent components, so also just did that, but now everything looks good! Let's merge it in :D. Congratulations on your first merged PR :)

tongxin97 · 2017-10-05T21:36:54Z

thanks @emilgoldsmith! I didn't really pay attention to the parents components.. my bad. Will look out for it next time.

fix for #206, add scroll to author & article pages

7dea9bf

tongxin97 requested a review from emilgoldsmith as a code owner September 25, 2017 21:56

emilgoldsmith requested changes Sep 25, 2017

View reviewed changes

move hash scroll to admin-client.js, add support for Issues page

d3498a1

flakybot2077 added 2 commits September 27, 2017 14:36

revert to 17c8d7a, cancel the scroll fix

477d011

add material ui Dialog to admin articles page

6c19b95

tongxin97 commented Sep 27, 2017

View reviewed changes

emilgoldsmith requested changes Sep 27, 2017

View reviewed changes

fix submit issue, enable page redirect in articles page

804300f

tongxin97 changed the title ~~Fix for #206, scroll to author & article pages on admin site~~ Fix for #206, show authors, issues & articles editors in Dialog boxes on admin site Oct 2, 2017

emilgoldsmith requested changes Oct 2, 2017

View reviewed changes

fix small problems with Dialog and add to Authors & Issues page

651ec48

Removed redundant style object from EditorArticleController

dc3a2cc

emilgoldsmith requested changes Oct 4, 2017

View reviewed changes

flakybot2077 and others added 2 commits October 5, 2017 13:46

remove Dialog from Issues page

081fefe

Removed redundant Paper wrappers around modals

4b44343

emilgoldsmith approved these changes Oct 5, 2017

View reviewed changes

emilgoldsmith merged commit d30d13e into master Oct 5, 2017

emilgoldsmith deleted the editor-scroll branch October 5, 2017 20:26

Fix for #206, show authors, issues & articles editors in Dialog boxes on admin site #217

Fix for #206, show authors, issues & articles editors in Dialog boxes on admin site #217

Conversation

tongxin97 commented Sep 25, 2017 • edited

emilgoldsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tongxin97 commented Sep 26, 2017

tongxin97 commented Sep 26, 2017

zanemountcastle commented Sep 26, 2017 • edited

emilgoldsmith commented Sep 26, 2017

tongxin97 commented Sep 26, 2017 • edited

emilgoldsmith commented Sep 26, 2017

tongxin97 commented Sep 27, 2017

tongxin97 Sep 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilgoldsmith commented Sep 27, 2017

tongxin97 commented Sep 27, 2017

emilgoldsmith commented Sep 27, 2017

tongxin97 commented Sep 27, 2017

emilgoldsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tongxin97 commented Sep 28, 2017

emilgoldsmith commented Sep 28, 2017

emilgoldsmith commented Sep 28, 2017

tongxin97 commented Sep 28, 2017

emilgoldsmith commented Sep 28, 2017

zanemountcastle commented Oct 1, 2017

tongxin97 commented Oct 2, 2017 • edited

tongxin97 commented Oct 2, 2017 • edited

emilgoldsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilgoldsmith commented Oct 2, 2017

emilgoldsmith commented Oct 2, 2017

tongxin97 commented Oct 3, 2017

emilgoldsmith commented Oct 3, 2017

zanemountcastle commented Oct 3, 2017

tongxin97 commented Oct 4, 2017

emilgoldsmith left a comment

Choose a reason for hiding this comment

zanemountcastle commented Oct 5, 2017 • edited

emilgoldsmith commented Oct 5, 2017

emilgoldsmith commented Oct 5, 2017

tongxin97 commented Oct 5, 2017

tongxin97 commented Sep 25, 2017 •

edited

zanemountcastle commented Sep 26, 2017 •

edited

tongxin97 commented Sep 26, 2017 •

edited

tongxin97 Sep 27, 2017 •

edited

tongxin97 commented Oct 2, 2017 •

edited

tongxin97 commented Oct 2, 2017 •

edited

zanemountcastle commented Oct 5, 2017 •

edited