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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Making manager more mobile friendly #12776

Closed
wants to merge 5 commits into
base: 2.4.x
from

Conversation

Projects
None yet
@jpdevries
Contributor

jpdevries commented Nov 21, 2015

What does it do ?

  • Gets the fixed and absolute positioned silliness out of the way on
    small screens by combatting ExtJS' nature of aggressively preventing usability on small viewports
  • fights 馃敟 with 馃敟
  • Makes the left hand tree more responsive (can expand and collapse with a button at the bottom of the page)
  • Make the resource edit screen mobile friendly

Why is it needed ?

So it is more feasible to navigate the manager and edit resources on mobile devices.

Related issue(s)/PR(s)

n/a

Before

After

@OptimusCrime

This comment has been minimized.

Contributor

OptimusCrime commented Nov 21, 2015

Pretty sure you should not include the rendered .css file. Other than that, awesome work!

@pixelchutes

This comment has been minimized.

Collaborator

pixelchutes commented Nov 21, 2015

This looks awesome, JP!

this._loadActionButtons();
console.log(document.getElementById('modx-container').innerHTML);

This comment has been minimized.

@pixelchutes

pixelchutes Nov 21, 2015

Collaborator

Any reason this shouldn't be removed?

This comment has been minimized.

@jpdevries

jpdevries Nov 21, 2015

Contributor

Lol whoops! That's what I get for submitting a pr during a MODXpo lecture. I'll fix it.聽


Sent from Mailbox

On Sat, Nov 21, 2015 at 11:20 PM, Mike Reid notifications@github.com
wrote:

 this._loadActionButtons();
  • console.log(document.getElementById('modx-container').innerHTML);
    Any reason this shouldn't be removed?

Reply to this email directly or view it on GitHub:
https://github.com/modxcms/revolution/pull/12776/files#r45551647

@jpdevries jpdevries force-pushed the jpdevries:responsive-header branch from 99883f8 to 4a4ca38 Nov 21, 2015

@jpdevries

This comment has been minimized.

Contributor

jpdevries commented Nov 21, 2015

@OptimusCrime AFAIK it is part of the workflow to build the files before committing. When conflicts happen with CSS Integrators can just re-run grunt build to solve them.

I removed the console.log statement and cleaned up by squashing the commits into one

@Mark-H

This comment has been minimized.

Collaborator

Mark-H commented Nov 22, 2015

@jpdevries Actually @OptimusCrime is right - to prevent conflicts from occurring it's better if the compiled CSS is not included in pull requests, the integrators will take care of updating that as things get merged. It's okay though, can sort it out either way.

Could you update your PR with the template from https://github.com/modxcms/revolution/blob/2.x/CONTRIBUTING.md#template-1? Gotta set the right example ;)

@jpdevries jpdevries force-pushed the jpdevries:responsive-header branch from 4a4ca38 to 8903ee2 Nov 22, 2015

@jpdevries

This comment has been minimized.

Contributor

jpdevries commented Nov 22, 2015

@Mark-H Oh I see, wasn't aware of that. done and done, i pulled the css out too

@Mark-H

This comment has been minimized.

Collaborator

Mark-H commented Nov 22, 2015

Thanks man :) This looks really good from your screenshot, but it looks like it might need some further tweaks. Let's look at this together tomorrow?

screen shot 2015-11-22 at 01 08 48

@jpdevries

This comment has been minimized.

Contributor

jpdevries commented Nov 22, 2015

Huh, ya i am not sure what is going on there. I originally branched off develop, but submitted the pr to 2.4.x. Not sure if that could have anything to do with it or not.聽


Sent from Mailbox

On Sun, Nov 22, 2015 at 1:09 AM, Mark Hamstra notifications@github.com
wrote:

Thanks man :) This looks really good from your screenshot, but it looks like it might need some further tweaks. Let's look at this together tomorrow?

screen shot 2015-11-22 at 01 08 48

Reply to this email directly or view it on GitHub:
#12776 (comment)

@Mark-H

This comment has been minimized.

Collaborator

Mark-H commented Nov 22, 2015

That could be related for sure, let's find out tomorrow, could also be an issue with my dev environment as I just started a fresh one.

@jpdevries

This comment has been minimized.

Contributor

jpdevries commented Nov 22, 2015

I reproduced it kind of but then after I cleared cache and refreshed the page it looks right again. Ya let's look at it tomorrow. It would be nice to try and figure out a way to format the uber bar too and maybe even re order the preview button (using flex-order) so it is last and big like the help button is now.聽


Sent from Mailbox

On Sun, Nov 22, 2015 at 1:19 AM, Mark Hamstra notifications@github.com
wrote:

That could be related for sure, let's find out tomorrow, could also be an issue with my dev environment as I just started a fresh one.

Reply to this email directly or view it on GitHub:
#12776 (comment)

@mindeffects

This comment has been minimized.

Contributor

mindeffects commented Nov 22, 2015

Great idea for a PR, @jpdevries!
But why not kill those vertical lines in the menu bar at all? They are not needed for anything and are already gone on the right side of the manu bar.

Making header more mobile friendly
Gets the fixed and absolute positioned silliness out of the way on
small screens

@jpdevries jpdevries force-pushed the jpdevries:responsive-header branch from 8903ee2 to ed1b82f Nov 22, 2015

@jpdevries

This comment has been minimized.

Contributor

jpdevries commented Nov 22, 2015

@mindeffects i'm not opposed to removing the lines, just mostly been focusing on responsive layout with this PR.

@Mark-H I think I know what is going on. The layout is still messed up if the tree is open

Tree Open 馃槩

Tree Closed 馃槃

@jpdevries

This comment has been minimized.

Contributor

jpdevries commented Nov 22, 2015

With a bit more work I managed to get the lefthand sidebar (tree) to be more supported on mobile.

It is kind of weird how you expand and collapse the tree, but I'm making do with what I can. If you scroll down to the bottom of the page there is a button to expand / collapse the tree

Haven't pushed yet but will soon.

@exside

This comment has been minimized.

Contributor

exside commented Nov 22, 2015

馃憤

@jpdevries

This comment has been minimized.

Contributor

jpdevries commented Nov 22, 2015

I intended to only work on the header (hints the responsive-header branch name) but figured I'd try and work my way down the page. How about making the content entry area a bit more mobile friendly?

Also made the settings tab more mobile friendly

@jpdevries jpdevries changed the title from Making header more mobile friendly to Making manager more mobile friendly Nov 22, 2015

@mindeffects

This comment has been minimized.

Contributor

mindeffects commented Nov 22, 2015

Cool!

Make Manager Edit page more mobile friendly
woohoo
Content tab
Settings tab
@jpdevries

This comment has been minimized.

Contributor

jpdevries commented Nov 22, 2015

Just updated the expand / collapse sidebar button to be responsive.

It鈥檚 a little weird because the component is all the way at the bottom
of the page. Ideally it would be just beneath the tree sidebar itself,
but we haven鈥檛 been able to figure out how to change where the
component is ordered within the markup so this is the best we can do
for now

@@ -104,7 +104,7 @@ Ext.extend(MODx.Component,Ext.Component,{
return true;
}
});
Ext.reg('modx-component',MODx.Component);

This comment has been minimized.

@Mark-H

Mark-H Nov 22, 2015

Collaborator

Is there any particular reason you moved this line down to line 359?

This comment has been minimized.

@jpdevries

jpdevries Nov 22, 2015

Contributor

that is from when I was trying to figure out how to change the order of where the action buttons where on the page, but I do't think all those changes to modx.component.js are needed. I'll try removing some of them

@jpdevries jpdevries force-pushed the jpdevries:responsive-header branch 2 times, most recently from a567759 to 36e7dea Nov 22, 2015

@Mark-H Mark-H added this to the v2.5.0-pl milestone Nov 22, 2015

@jpdevries

This comment has been minimized.

Contributor

jpdevries commented Nov 22, 2015

This PR does change where the MODX action bars get rendered to (so they can be before the main area) so I suppose that could cause some edge cases but all it is really doing is giving it another wrapper. If people are selecting the actions box by the id to do things like add their own buttons all that should work fine.
Either way, I suppose targeting 2.5 is good to be safe.

@jpdevries jpdevries force-pushed the jpdevries:responsive-header branch from 19e2ed7 to 39d051e Nov 24, 2015

@christianseel

This comment has been minimized.

Contributor

christianseel commented Nov 24, 2015

Fantastic job JP! 馃帀 馃憤

@mindeffects

This comment has been minimized.

Contributor

mindeffects commented Nov 25, 2015

Awesome!
I hope this PR makes it into 2.4.3!

@jpdevries

This comment has been minimized.

Contributor

jpdevries commented Nov 25, 2015

I hope this PR makes it into 2.4.3!

@mindeffects I did as well but it has been slated for 2.5.0 mostly because it moves where the actions toolbar is in the DOM. It still uses the same exact id, so I see no reason why this would break anything.

I intend to install 2.4.3 built from my fork so I can start my clients on it early. I can share the zip ;)

@eighthday

This comment has been minimized.

eighthday commented Nov 26, 2015

amazing! never thought this would come before modx 3

@jaygilmore

This comment has been minimized.

Member

jaygilmore commented Nov 26, 2015

Squee!

@rtripault

This comment has been minimized.

Collaborator

rtripault commented Nov 26, 2015

Absolutely awesome work & effort! Huge 馃憤 from here!

My only concern is about code comments (as in many other core code places, unfortunately).
SASS allows comments, which could be "removed" when compiling, so i would love having loads of those.

It's hard for other maintainers/contributors to understand the intention, or even what's supposed to be targeted, without comments.

@jpdevries

This comment has been minimized.

Contributor

jpdevries commented Nov 26, 2015

@rtripault glad you like it!

The Sass is certainly nasty. Overly specific selectors and !important declarations are needed to override the inline styling of ExtJS. I could certainly add some comments to help organize things. Also, I would like to adjust the styles for certain components but would need some help from an ExtJS contributor that could help add a few classes in certain areas and maybe change the order of a couple of things in the DOM if possible.

@pixelchutes

This comment has been minimized.

Collaborator

pixelchutes commented Nov 28, 2015

@jpdevries I haven't had a chance to test this yet, but one observation on the "Uberbar Focused" and "Uberbar Results" images above:

It seems to have regressed the 'up arrow' visual connector, which previously was designed to align with the magnifying glass at each width / breakpoint:

Before

Notice at the end (once resized smaller)
searchbar

@HarveyEV

This comment has been minimized.

HarveyEV commented Nov 28, 2015

Awesomesauce! Can't wait for 2.5. 馃憤

@jpdevries

This comment has been minimized.

Contributor

jpdevries commented Nov 28, 2015

@pixelchutes good call on the arrow that should be an easy fix

more mobile enhancements (momo)
Import HTML
Import Static HTML
Resource Groups
Content Types
Media Sources
Installer
Manager Customizations
Dashboard
Contexts
Menus
ACLs
Property Sets
Lexicon Management
Namespace

@jpdevries jpdevries force-pushed the jpdevries:responsive-header branch from 39d051e to af66ce7 Nov 28, 2015

@jpdevries

This comment has been minimized.

Contributor

jpdevries commented Nov 28, 2015

updated

@Mark-H

This comment has been minimized.

Collaborator

Mark-H commented Dec 1, 2015

This has been merged into 2.x for inclusion in 2.5, thank you @jpdevries for making MODX more usable!!

@Mark-H Mark-H closed this Dec 1, 2015

@jpdevries

This comment has been minimized.

Contributor

jpdevries commented Dec 1, 2015

@Mark-H what can I say i can't 馃挙 on 鉁堬笍
Thanks for the merge and Happy December 馃槃

@noahlearner

This comment has been minimized.

noahlearner commented Dec 2, 2015

Will this work with touch? A big part of how the manager works depends on right clicking menu items. How will we do this on mobile device / tablet?

@Mark-H

This comment has been minimized.

Collaborator

Mark-H commented Dec 2, 2015

@noahlearner This pull request doesn't change that, this just makes sure that you're actually able of navigating through the manager without wanting to poke your eyes out. :) Various mobile devices/tablets have different ways of doing right clicks.. long or force tabs typically.

I'm sure there'll be further improvements in the future specifically to target the right clicks, but this one is not that yet.

@noahlearner

This comment has been minimized.

noahlearner commented Dec 2, 2015

Will the solution look like a javascript solution to listen for a hold down to map to right click?

Kind of like the solution on: http://stackoverflow.com/questions/2625210/long-press-in-javascript

or the http://www.jquery-plugins.info/longclick-00014475.htm

@rtripault

This comment has been minimized.

Collaborator

rtripault commented on manager/assets/modext/core/modx.component.js in ed1b82f Dec 3, 2015

This could break existing manager themes having their own header.tpl Smarty template.
While chances are there aren't so many custom manager themes, probably worth a "bold" notice/warning in the release notes

This comment has been minimized.

Collaborator

Mark-H replied Dec 3, 2015

Fix proposed in #12798

@rtripault

This comment has been minimized.

Collaborator

rtripault commented on _build/templates/default/sass/index.scss in b9d4e74 Dec 3, 2015

This breaks any content scrolling on desktop (Chrome/Linux)

This comment has been minimized.

Collaborator

Mark-H replied Dec 3, 2015

Proposed fix in #12799

Mark-H added a commit to Mark-H/revolution that referenced this pull request Dec 3, 2015

Ensure compatibility of more mobile-friendly manager with custom mana鈥
鈥er themes

The more mobile-friendly manager introduced in modxcms#12776 relies on a new div with id modx-action-buttons-container being present in the header.tpl, however with custom manager themes that have their own header.tpl file that may not always be the case. With this commit in place, it ensures the action buttons are still inserted into the page (albeit in the wrong page), rather than throwing a js error if it can't find the new div.
@rtripault

This comment has been minimized.

Collaborator

rtripault commented Dec 5, 2015

Just rising an "slightly boring" issue :

  • change your browser viewport to a "mobile" one, ie. 615px (the tree is now at the top and its width is ~600px)
  • restore to a "desktop width", notice the tree is still around 600px, with an horizontal scroollbar at the bottom

@jpdevries jpdevries referenced this pull request Jan 19, 2016

Closed

"MODX" Next Roadmap? #12863

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