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

New revised user experience - Refinery Glass #3071

Closed
wants to merge 17 commits into from

Conversation

stefanspicer
Copy link
Contributor

This replaces #2999. It has been rebased and includes a few more features, fixes and refactored JS.

Here are a few highlights: https://youtu.be/8nZgeF-QwMo
Here is a demo site I set up. Go and try it out: http://refineryglass.com

This PR works in conjunction with refinery/refinerycms-authentication-devise#14

Development of Refinery Glass started in Oct 2014 and after over 1000 commits (that I've condensed into a few) we've launched this new refinery on 5 production websites.

Main Features of Refinery Glass

  • New look - main menu, sidebars, icons, contextual menu (#actions), responsive
  • glass editor to replace wymeditor with grande.js integrated with refinery - edit content in place as it will show live
  • Ajax is used for forms to enhance the user experience
  • Invite admin - they activate and create a password. A professional "Welcome to your CMS" experience.

Still a lot left to do

  • I've marked a bunch of tests as skipped, the reason shows my explanation. This is a good place to start to get things up to speed
  • The editor is very simple, doesn't have some of the features of wymeditor
  • Multiple locales on pages needs to be re-integrated into the interface, and re-thought through
  • I have a list that we need to prioritize, the above are the biggest things.

stefanspicer and others added 11 commits November 13, 2015 13:29
Includes (but not limited to):
 - Invite user, activates account from link in their email
 - New 'glass' look (css), getting closer to responsive design
 - New 'glass' editor using grande.js - uses contenteditable attr
 - AJAX forms & JS validation
 - We feel its Okay to be quite limiting of browsers here to cut down on
   testing time and be able to use new browser features.  For the actual
   clients' sites it is probably good to support a wider range of
   browsers
 - Plugins have a default order
 - New plugins specifiy a "position" number (0-100)
 - Refinery::Core.config.backend_menu_positions can override these
   positions
 - 0 specifies hidden, just so there is no menu item for the plugin
 - Found this useful for creating a thank-you page in a seed.rb, then
   finding it by its "identifier" in the controller to redirect to it
 - This is an attribute set only in code, and read only in code - so we
   don't rely on, for example, redirect_url not to be changed by the
   admin
 - Single click to upload (open browse for file window)
 - Progress bar while uploading
 - Image shown in place
 - It flattens the HTML and strips most attributes
 - search for each word in the query (split on space)
@bricesanchez
Copy link
Member

That's a really cool improvement.

Should we start using it now and check if all extensions works well ?

@parndt Could we target 4.0 for this PR ?

@stefanspicer
Copy link
Contributor Author

What do you mean @bricesanchez by "start using it now"? We are using it in production, but that only means it works within the way we use refinery.

As to the extenstions, it is not a "check if they work well" but "port them over to the new html conventions" if that makes sense. I recommend dropping support for Inquiries, and instead make sure the engine generator outputs a good inquiries engine. I have a blog plugin started, it has some feature overlap with the current blog, but it also differs in many ways. We can potentially come up with something in between. I have never installed any other extensions, so I can't speak to them.

@bricesanchez
Copy link
Member

By start using it now i mean in your opinion is it ready to be merged on master even if it remains works to do ?

I like your idea to drop the support of inquiries and improve the form generator.

@stefanspicer
Copy link
Contributor Author

To be honest I don't know what it really means to have something merged into master. Please note that there are a lot of skipped (essentially failing) tests. I skipped them so that I could put a comment in and describe what is going on. Some are just because I couldn't get the test to pass when I think it should. With less automated tests it means we rely on more manual tests (which are all run withing our environment).

Back to the questions, I don't have a good idea who is running off of master, so it is really a question for them I guess. We have dropped functionality with this update so I guess worst case scenario it is a scramble to re-build the missing functionality and fix the bugs that are still lurking.

@anitagraham
Copy link
Contributor

Why dropping inquiries?

@anitagraham
Copy link
Contributor

Looks good.

@parndt
Copy link
Member

parndt commented Nov 15, 2015

I like the idea of master branch always being working, but unreleased and not necessarily supported, software

@stefanspicer
Copy link
Contributor Author

@anitagraham, my reasons for dropping support for inquiries:

  • I found I always spent more time overriding things I needed to change (add a field, change the mailer, ...) than if all the code was right in front of me in vendor/extensions
  • It is helping developers get into the "refinery way" faster to make the "inquiries extension" instead just be a list of commands like:
    • rails g refinery:engine inquiry ...
    • rails g refinery:inquries
    • rake db:migrate
    • rake db:seed
    • ...
  • refinerycms-inquiries is just not different enough from the generator to warrant a separate extension

@stefanspicer
Copy link
Contributor Author

@parndt, I can confirm that this PR is working. But I can see people potentially thinking it isn't "working" because of the missing features. It adds features and removes some. The main features removed at this point on my list are:

  • images library (this is intentionally removed)
  • resources (I'll be adding a working version of this in very soon)
  • page locales
  • i18n - I'm pretty sure we've introduced a ton of hard coded English in the admin interface
  • html editor - missing features, some intentional like an html view

@parndt
Copy link
Member

parndt commented Nov 15, 2015

@stefanspicer yeah, I think that's what I meant. Things like i18n are actually quite a big deal as we have users using many different languages

@stefanspicer
Copy link
Contributor Author

Yes, I thought that would be a big deal. And the reason it was left out is simply that we have had no need (up to this point) for anything other than English, so it naturally sort of slips behind.

@JulianNacci
Copy link

Refinery Glass Authentication I18n will be finalized today, we will dig into Refinery I18n this week with a colleague. Will make a PR on your repo soon @stefanspicer.

Im here speaking about hardcoded back-office english, we will fix keys and ensure :en and :fr translations are complete, but wont dig in other yml.

About content I18n feature, it is apparently disabled and I have no idea about how it works. @stefanspicer @bricesanchez @parndt can this be worked out quite easily ?

@JulianNacci
Copy link

I18n will be a real challenge as we spoted hardcoded english in js, css (?!), views and controllers.

@stefanspicer
Copy link
Contributor Author

@JulianNacci, yes, not a simple task. I want to support you in any way needed. If you want to put a list together of any tough ones, that might be helpful:

  • Views - should be straightforward (just a lot of grunt work)
  • Controllers - is this mainly error messages in json responses? I wonder if json can be moved into a 'json' view or something to solve that.
  • js - there's probably way more html in the js files than there should be. I've moved a lot out into views, but this job probably just needs to completed.
  • css - is it more than these 2 "content" lines: "New paragraph..." and "Move Here"? We should move these into views and JS can store it in attach them as a data attribute to any element its needed (for css to pick up) - if you'd like us to handle this, I can make this change.

@stefanspicer
Copy link
Contributor Author

Thank you @parndt, it's not that frustrating, and is something I've attempted to do already. I actually have 9 other (fairly small) changes that I've extracted out of this in preparation to submit them each individually.

I've attempted to make each of the current 18 commits a separate "feature" of their own that follow the current tip of master. The only exception is the first one 30134e8 New 'glass' interface for refinery. That is explained further in my comment from back in June. Instead of reviewing the code directly, it may be helpful to review my comment. Instead of me going and attempting to break that one apart, I've put the time into breaking it apart in theory (in that comment). If we can work together to come up with a plan, I can put the time into breaking it into the PRs that make sense.

Oh, and I forgot to mention that the glass-3-0-2 branch is what @bricesanchez and I are currently working on for the next PR to replace this one. It has fixes and features from the past 3 months in there too, and has been rebased onto master.

@stefanspicer
Copy link
Contributor Author

@parndt shall we work on a plan together for how this should be broken out? Or @bricesanchez as you dig into this PR maybe you'll have some more insight and can speak to this?

@parndt
Copy link
Member

parndt commented Mar 30, 2016

I am supremely low on time right now, sorry. Feel free to ask me directed questions though and I'll do my best!

@bricesanchez
Copy link
Member

@stefanspicer I've dig into this PR and i'm sure the result of your work could be the future of Refinery CMS.

I'm in favor of splitting this PR to be able to make a good review and reduce the number of regressions or bugs.

In the other way, i'm interested to try it as it on a new project and see what happens.

I've also seen that there is nearly no changes in controllers or models, only views, css and JS and i'm asking myself if we could use it as an engine before merging it.

If we split the PR, we could make this plan :

  1. @stefanspicer : Provide a PR to change layout of the admin (sidebar with plugin menu and general CSS)
    @refinery/collaborators : Be sure that all specs are running and all legacy views (pages, resources, images) are working.
  2. @stefanspicer : Provide a PR to change the admin views
    @refinery/collaborators : Be sure that all specs are running and all legacy views (resources, images) are working.
  3. @stefanspicer : Provide a PR to add live editor feature
    @refinery/collaborators : Be sure that all specs are running and all legacy views (resources, images) are working.

@stefanspicer
Copy link
Contributor Author

I think your first comment about the future of refinery is actually my biggest question. Before I go and put even more hours into this, I need to see some people to agree that this is the future of refinery. My team and I see how refinery could become the best CMS for developers to very efficiently provide a custom solution while surprising and wow'ing their client at the same time.

If you are willing, I think the 2-4 hours you spend trying this out on a new project will give you more insight than the 12-24 hours I could put in splitting this out. I would even gladly work with you on getting it set up and walking through some things as documentation is currently lacking.

This can definitely be an engine, but I don't think this is the way to go. My pain in that was that it started out as an engine and was one up until Apr 2015 when I put the effort in to clean it up and bring it into the refinery core while being careful not to revert all the changes that happened while we were off in our own fork.

So if you're okay with it, I'll wait until you have at least attempted to use this on a project and we'll probably come out of that with a more specific (and hopefully better) plan on how to merge this PR. Let me know if/when you want me to jump in and help out on your project.

@bricesanchez
Copy link
Member

bricesanchez commented Apr 21, 2016

I've tried your PR on an existing project in version 3.0.2 and on a dummy project.

This is my thoughts :

Pros :

  • The new look and feel is nice!
  • The live editor is a very interesting feature.

Cons :

  • This PR is too big to be reviewed and merged. The review process is really important because after the merge Refinery contributors will endorse your work and they will have to deal with the issues and the maintenance. So they have to be confortable with your work before to merge it. Split this PR into incrementable PRs is a good start in order to move forward and to be reviewed.
  • There is no support of legacy CSS but my PR could help you soon Split and tidy up stylesheets #3165 (it will be easier to load few CSS).
  • a current project can't be easily updated without breaking a lot of UI.

Questions :

  • Why don't you use turbolinks ?

@parndt
Copy link
Member

parndt commented Apr 21, 2016

Why don't you use turbolinks ?

We don't either 😉

@bricesanchez
Copy link
Member

@parndt We don't because we haven't rebase, fix and merge this PR : #2732 :D

@stefanspicer
Copy link
Contributor Author

I'm glad you like the look. It's had many iterations. This live editor idea is the future of the web and refinery has an opportunity here to lead on certain aspects if we want to take this on. Quick note about turbolinks: We're planning on going that way, just trying to keep changes as small as possible and modifying the request.xhr? checks seemed like too much at this point.

Updating a project to this version is going to be somewhat painful regardless of how we slice it. I think creating good guides will help with this. For example, you have a legacy FAQ engine. Regenerating it with the same parameters and then using git add -p to add only the admin views is pretty simple to gets you a long way. Then you just need to re-add any admin interface features. But I agree with you, upgrading is not simple. The question is, are we going to make it worthwhile for people to upgrade and do that work? I think if we sell it right, people will be all over it. But that's my opinion.

On to splitting and merging incrementally. Let's go one chunk at a time building upon the current 3.0.2. What is the first PR chunk now that you've dug in a bit? Same as before @bricesanchez "change layout of the admin (sidebar with plugin menu and general CSS)"

Outcome of this:
refinery-pr1

@stefanspicer
Copy link
Contributor Author

Is that the plan @bricesanchez? It may make more sense to take the menu and index views as the first chunk.

@parndt
Copy link
Member

parndt commented Apr 23, 2016

Seems like it's using the old icons? Otherwise looks generally good.

@bricesanchez
Copy link
Member

Yes @stefanspicer :)

@stefanspicer stefanspicer mentioned this pull request Apr 25, 2016
@stefanspicer
Copy link
Contributor Author

Thank you for merging #3167 @bricesanchez. That's great. On to the next chunk? (or "shard"?)

I have 90% prepared the admin/users/index view. Shall I submit that PR next?

@bricesanchez
Copy link
Member

@stefanspicer You're welcome!

admin/users/index should be a feature for refinerycms-authentication-devise because there is no User concept in the core of the project.

We should stay concentrate on this repo before extensions repos.

We could work on the layout/views CSS styles for backend (pages, images, resources).

No need to work on the inline editing feature at this time.

What do you think?

@stefanspicer
Copy link
Contributor Author

I'd prefer to do the admin/users/index view next just because it is most of the way there already. But I'm OK to switch gears. Even though refinerycms-authentication-devise has been technically broken out into an extension, I still see it as very much apart of the core.

So my fist choice is admin/users/index. Second choice is to do admin/resources/index next. I want to do a simpler index first, then move to admin/pages/index. Let me know, and I'll get to work.

@bricesanchez
Copy link
Member

@stefanspicer I'm ok with that, i will help you to get these PRs merged :)

@parndt
Copy link
Member

parndt commented Jun 2, 2016

Yes, users are still sort of part of the core, but not the devise implementation.. will be interested to see what you come up with anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants