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

Settings UI: Put advanced settings in foldable cards #6550

Merged

Conversation

dereksmart
Copy link
Member

@dereksmart dereksmart commented Mar 2, 2017

Fixes #6507
Fixes #6503

❗️ Make sure to yarn distclean && yarn build to get the newest dops-components❗️

screen shot 2017-03-02 at 10 20 35 am

screen shot 2017-03-02 at 10 20 45 am

To Test:
Make sure forms and saving works properly.
Make sure it looks good.

@dereksmart dereksmart added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] Needs Design Review Design has been added. Needs a review! [Status] Needs Review To request a review from Crew. Label will be renamed soon. Admin Page React-powered dashboard under the Jetpack menu labels Mar 2, 2017
@dereksmart dereksmart added this to the Settings UI milestone Mar 2, 2017
@dereksmart dereksmart added this to Review needed in Settings UI Mar 2, 2017
@eliorivero eliorivero added [Status] In Progress [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Status] In Progress [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 2, 2017
Copy link
Contributor

@eliorivero eliorivero left a comment

Choose a reason for hiding this comment

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

These should be grayed out and disabled in dev mode, or maybe even not visible:

captura de pantalla 2017-03-02 a las 15 46 08

captura de pantalla 2017-03-02 a las 15 46 17

@dereksmart
Copy link
Member Author

Turns out the actual designs we're going for is this
screen shot 2017-03-02 at 9 09 18 am

In progress for now.

@dereksmart dereksmart removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Mar 2, 2017
@eliorivero
Copy link
Contributor

Makes sense. The advanced settings looked integrated in the Stats card, as part of the card, but they are a bit confusing in the Composing card since it's not clear if they're advanced options for all the card or just one setting.
I wonder how it looks in Stats. Because in Composing now it's clear it's for AtD, but Stats has two options and adding the chevron for one of them might look like it's only for that option and would be confusing.

@eliorivero eliorivero dismissed their stale review March 3, 2017 12:25

PR is in progress

@dereksmart
Copy link
Member Author

I wonder how it looks in Stats. Because in Composing now it's clear it's for AtD, but Stats has two options and adding the chevron for one of them might look like it's only for that option and would be confusing.

I was thinking this too. @MichaelArestad?

@zinigor zinigor moved this from Review needed to In progress in Settings UI Mar 6, 2017
@MichaelArestad
Copy link
Contributor

MichaelArestad commented Mar 6, 2017

@dereksmart for stats, we will put all of the settings in the foldable part. There will just be a normal description and the chevron for stats. Does that make sense?

Maybe even just a label that says "Site stats"

@dereksmart
Copy link
Member Author

Will there be a toggle for activating the module? Can you please just whip up a quick screenshot/mock?

@MichaelArestad
Copy link
Contributor

@dereksmart no toggle. There already is no toggle for this module.

Something like this (but the JP version)

image

@dereksmart dereksmart force-pushed the add/advanced-settings-foldable branch 2 times, most recently from 1e173c9 to d41588f Compare March 8, 2017 15:45
@eliorivero eliorivero force-pushed the add/advanced-settings-foldable branch from 8aea063 to c5fa71b Compare March 9, 2017 18:15
@eliorivero eliorivero force-pushed the add/advanced-settings-foldable branch from 090ccb3 to 79b6b32 Compare March 9, 2017 20:56
@eliorivero eliorivero moved this from In progress to Review needed in Settings UI Mar 9, 2017
@eliorivero eliorivero force-pushed the add/advanced-settings-foldable branch from 79b6b32 to c969263 Compare March 9, 2017 21:10
@eliorivero
Copy link
Contributor

This PR is now updated and AtD and Stats are now in foldable cards:

captura de pantalla 2017-03-09 a las 17 59 38

captura de pantalla 2017-03-09 a las 17 59 52

captura de pantalla 2017-03-09 a las 18 11 14

captura de pantalla 2017-03-09 a las 18 11 01

@eliorivero eliorivero added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Mar 9, 2017
header={ __( 'Site stats' ) }
module="stats">
className={ classNames( 'jp-foldable-settings-standalone', { 'jp-foldable-settings-disable': unavailableInDevMode } ) }
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this one makes sense to have a clickableHeader

@dereksmart
Copy link
Member Author

Pushed one small change to make stats card header clickable. I think this is good. @MichaelArestad what say you?

@MichaelArestad
Copy link
Contributor

This is great! Thank you! Ship this big ol' thang!

@MichaelArestad MichaelArestad removed the [Status] Needs Design Review Design has been added. Needs a review! label Mar 9, 2017
Copy link
Contributor

@MichaelArestad MichaelArestad left a comment

Choose a reason for hiding this comment

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

Basically, just wrap any new pixel value in the rem() function. Other than that, the code looks fine.

}
.form-toggle__switch {
float: left;
margin-top: 2px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs rem()

}

.jp-module-settings__learn-more {
right: 2px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs rem()

margin-bottom: rem( 24px );
}
.form-toggle__label {
margin-top: 4px;
Copy link
Contributor

Choose a reason for hiding this comment

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

both of these need rem()

@dereksmart dereksmart merged commit 30fca6b into feature/settings-overhaul Mar 9, 2017
@dereksmart dereksmart deleted the add/advanced-settings-foldable branch March 9, 2017 21:57
@dereksmart dereksmart removed the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Mar 9, 2017
@dereksmart dereksmart moved this from Review needed to Done in Settings UI Mar 9, 2017
eliorivero pushed a commit that referenced this pull request Mar 13, 2017
* Put site stats advanced settings in foldable card

* ATD: put advanced settings in foldable card

* fix eslint warnings/errors

* ATD settings foldable

* put all of site stats settings in foldable card

* Settings UI: update foldable card and its settings for After the Deadline

* Settings UI: finish putting Stats into standalone foldable card

* Site Stats foldable card make header clickable

* wrap new pixel values in rem()
eliorivero pushed a commit that referenced this pull request Mar 14, 2017
* Put site stats advanced settings in foldable card

* ATD: put advanced settings in foldable card

* fix eslint warnings/errors

* ATD settings foldable

* put all of site stats settings in foldable card

* Settings UI: update foldable card and its settings for After the Deadline

* Settings UI: finish putting Stats into standalone foldable card

* Site Stats foldable card make header clickable

* wrap new pixel values in rem()
anomiex added a commit that referenced this pull request May 10, 2022
Did a pass over _inc/client/components looking for things that weren't
used anywhere, and found a bunch:

* components/data/query-connect-url - Last use removed in #8014
* components/data/query-connection-status - Last use removed in 62e9ab0
* components/data/query-modules - Last use removed in bfc40ad
* components/data/query-plugin-updates - Last use removed in #17003
* components/data/query-site-products - Last use removed in #21594
* components/form/* - Didn't check for last use, too many bits. But it
  looks like the `formsy-react` package many of these depended on wasn't
  even installed since #8208.
* components/inline-expand - Last use removed in #6550
* components/jetpack-dialogue - Last use removed in #16518
* components/jetpack-logo - Last use removed in #20148
* components/jetpack-termination-dialog - Last use removed in #21048
* components/module-settings/index.jsx - Last use removed in #10644
* components/module-settings/inline-module-toggle.jsx - Last use removed
  in #12118
* components/screen-reader-text - Last use removed in #18843
* components/settings - Last use removed in 26315e1, I think
* components/tags-input - Last use removed in #11772

Then there were a few more that were only used from some of the above:

* components/data/query-connected-plugins
* components/module-settings/form-components.jsx
* components/multiple-choice-question
* components/setting-toggle
anomiex added a commit that referenced this pull request May 10, 2022
Did a pass over _inc/client/components looking for things that weren't
used anywhere, and found a bunch:

* components/data/query-connect-url - Last use removed in #8014
* components/data/query-connection-status - Last use removed in 62e9ab0
* components/data/query-modules - Last use removed in bfc40ad
* components/data/query-plugin-updates - Last use removed in #17003
* components/data/query-site-products - Last use removed in #21594
* components/form/* - Didn't check for last use, too many bits. But it
  looks like the `formsy-react` package many of these depended on wasn't
  even installed since #8208.
* components/inline-expand - Last use removed in #6550
* components/jetpack-dialogue - Last use removed in #16518
* components/jetpack-logo - Last use removed in #20148
* components/jetpack-termination-dialog - Last use removed in #21048
* components/module-settings/index.jsx - Last use removed in #10644
* components/module-settings/inline-module-toggle.jsx - Last use removed
  in #12118
* components/screen-reader-text - Last use removed in #18843
* components/settings - Last use removed in 26315e1, I think
* components/tags-input - Last use removed in #11772

Then there were a few more that were only used from some of the above:

* components/data/query-connected-plugins
* components/module-settings/form-components.jsx
* components/multiple-choice-question
* components/setting-toggle

Co-authored-by: Brandon Kraft <public@brandonkraft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin Page React-powered dashboard under the Jetpack menu [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants