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

Enhancements for flexbox on smaller screens #6222

Conversation

clarkepaul
Copy link
Contributor

Fixes #6164

Requires Framework, CMS, AssetAdmin, Site-config branches of features/4.0/responsive-improvements

Copy link
Member

@sminnee sminnee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of small changes.

@@ -80,6 +80,7 @@ class FormBuilderModal extends SilverStripeComponent {
* @returns {Promise}
*/
handleSubmit(data, action, submitFn) {
console.log(data, action, submitFn);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no.

this.handleBackClick = this.handleBackClick.bind(this);
}

handleBackClick(event) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If onBack isn't supplied, it will just have a no-op. I think we should throw an error if onBack isn't set and the button is clicked.

Alternatively, if onBack isn't supplied, remove the back button altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flamerohr ^ is something you can look at

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, I think removing the back button makes sense, gives the option of not having the back button then

@clarkepaul clarkepaul force-pushed the features/4.0/responsive-improvements branch from 368399e to a0f10c8 Compare October 27, 2016 23:39
@clarkepaul clarkepaul force-pushed the features/4.0/responsive-improvements branch from a0f10c8 to a5d3dcc Compare October 28, 2016 03:56
@tractorcow
Copy link
Contributor

tractorcow commented Oct 30, 2016

In the security section, reducing browser width removes the action button column. Perhaps it should remove all except the first / last columns?

image

can you make it work the same way that campaigns does? (drops from 4 columns to 2)

image

@tractorcow
Copy link
Contributor

I don't know if this is related to the current change but the security cancel button styling is weird.

image

@tractorcow
Copy link
Contributor

In the campaign section there is inconsistent placement for the "back" button.

When previewing a page with an available preview the back button appears as this:

image

However if the page has no available preview, the back button appears in another place.

image

@clarkepaul
Copy link
Contributor Author

I've fixed the list view, will push shortly.
My security cancel button looks fine, have you checked out the siteconfig branch too?

@clarkepaul
Copy link
Contributor Author

@tractorcow In regards to showing the first and last column in grid fields, the problem is that all grid fields are composed differently so you can't always say what is first or last (eg page list has toggle first). With the Campaigns grid field I know there are two sets of actions at the end so I can show them, I also know they fit at a mobile screen size where-as other actions are sometimes accompanied by text which can break the layout. Actions are available once you select an item and go deeper so we are not removing functionality.

@clarkepaul clarkepaul force-pushed the features/4.0/responsive-improvements branch from 59666c2 to 4cc6cc3 Compare October 31, 2016 00:06
@clarkepaul
Copy link
Contributor Author

It was a conscious decision to use text to support the back arrow in the Campaign preview because it doesn't sit next to the inner page navigation like breadcrumbs or page name. Not a big deal really if I did remove the text but I think its better to decide that when we introduce the rest of the preview editing actions and draft/publish toggle.

@tractorcow tractorcow merged commit 37cb351 into silverstripe:master Oct 31, 2016
@tractorcow tractorcow deleted the features/4.0/responsive-improvements branch October 31, 2016 01:17
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

Successfully merging this pull request may close these issues.

None yet

4 participants