Skip to content

Settings UI: update support links to match Calypso style #6430

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 21, 2017
Merged

Settings UI: update support links to match Calypso style #6430

merged 2 commits into from
Feb 21, 2017

Conversation

eliorivero
Copy link
Contributor

@eliorivero eliorivero commented Feb 16, 2017 •
edited
Loading

Fixes #6198

Changes proposed in this Pull Request:

  • update support links in SettingsGroup and FormFieldset components to match Calypso style

captura de pantalla 2017-02-16 a las 14 47 00

  • resolves the previous misalignment that existed in standalone toggles/support icons in Discussion > Comments

captura de pantalla 2017-02-16 a las 16 22 54

  • update its code to solve ESLint warnings

Testing instructions:

  • make sure all support links work correctly

@eliorivero eliorivero added Admin Page React-powered dashboard under the Jetpack menu [Status] Needs Review This PR is ready for review. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Feb 16, 2017
@eliorivero eliorivero added this to the Settings UI milestone Feb 16, 2017
@eliorivero eliorivero self-assigned this Feb 16, 2017
@eliorivero eliorivero changed the title Update support links to match Calypso style Settings UI: update support links to match Calypso style Feb 16, 2017
@eliorivero eliorivero force-pushed the update/support-links-calypso branch 3 times, most recently from 8c2822d to 7680e51 Compare February 16, 2017 18:25
@eliorivero eliorivero mentioned this pull request Feb 16, 2017
Closed
@eliorivero eliorivero force-pushed the update/support-links-calypso branch from 7680e51 to 0ea581b Compare February 16, 2017 19:07
@eliorivero eliorivero force-pushed the update/support-links-calypso branch from 0ea581b to d07615f Compare February 16, 2017 19:22
@beaulebens
Copy link
Member

In testing this, I found that the Pinterest link is actually 404ing now. I looked around and I think the best option is to point it at https://www.pinterest.com/settings.

I posted about that for Calypso over here.

Other than that, all the tooltips seem to be working nicely for me.

dereksmart
dereksmart reviewed Feb 20, 2017
View reviewed changes
_inc/client/components/settings-group/index.jsx Outdated
// Disable in Dev Mode
const module = props.module,
disableInDevMode = props.disableInDevMode && props.isUnavailableInDevMode( module.module );
let support = props.support
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's set this with defaultProps instead

@dereksmart
Copy link
Contributor

As discussed in Slack, and sort of outside the scope of this PR, I think we should be paying more attention to our custom components and making sure that we typecheck our props, and provide defaults for them when necessary.

MichaelArestad
MichaelArestad reviewed Feb 20, 2017
View reviewed changes
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.

Design-wise, other than my comment about the external link icon, this looks great.

_inc/client/components/settings-group/index.jsx Outdated
</Button>
<InfoPopover screenReaderText={ __( 'Learn more' ) }>
<ExternalLink
icon={ true }
Copy link
Contributor

Choose a reason for hiding this comment

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

We should get rid of the external link icon on these. Here's the issue for it: #6252

@eliorivero
Copy link
Contributor Author

@dereksmart updated to include prop default and type.

@MichaelArestad icon removed

@eliorivero
Copy link
Contributor Author

@beaulebens @dereksmart
I tested the link earlier today
https://pinterest.com/website/verify/
and indeed it wasn't working, but now it's working.

@beaulebens
Copy link
Member

If it's working for you guys, go ahead then, the rest of this is looking great.

I'm guessing maybe it's because I have a verified URL already.

zinigor
zinigor approved these changes Feb 21, 2017
View reviewed changes
Copy link
Contributor

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Looks good and works well!

@zinigor zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Feb 21, 2017
@eliorivero eliorivero merged commit 5257ca6 into feature/settings-overhaul Feb 21, 2017
@eliorivero eliorivero deleted the update/support-links-calypso branch February 21, 2017 14:31
@eliorivero eliorivero removed the [Status] Ready to Merge Go ahead, you can push that green button! label Feb 21, 2017
@eliorivero eliorivero mentioned this pull request Feb 24, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dereksmart dereksmart dereksmart left review comments

@beaulebens beaulebens Awaiting requested review from beaulebens

+2 more reviewers

@MichaelArestad MichaelArestad MichaelArestad left review comments

@zinigor zinigor zinigor approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@eliorivero eliorivero

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
None yet
Milestone
Settings UI
Development

Successfully merging this pull request may close these issues.
5 participants
@eliorivero @beaulebens @dereksmart @zinigor @MichaelArestad