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

Site Settings: Moves Site Address up to Site Profile card #1682

Merged
merged 1 commit into from
Dec 17, 2015

Conversation

alternatekev
Copy link
Contributor

Currently, Site Address is a little buried, and kind of in the wrong place. There's no real reason it should be grouped with Visibility instead of Site Profile:

screen shot 2015-12-16 at 9 29 58 am

This PR cleans up the whole Site Profile card and moves Site Address to a CompactCard-looking area (replicated with the component's current use of <fieldset />):

screen shot 2015-12-16 at 9 32 53 am

pinging @mikeshelton1503 @rickybanister

@alternatekev alternatekev added [Type] Enhancement [Feature] Site Settings All other general site settings. [Status] In Progress [Feature Group] Emails & Domains Features related to email integrations and domain management. labels Dec 16, 2015
@rickybanister
Copy link

This gave me an idea—I think the address should be the second item, right below the title. That mimics the Site card.

We should then put the blavatar interface to the left of those two fields and we create a sort of wysiwyg Site card editing interface.

The tag line would go below that in a separate compact card with language.

I'm not really sure visibility then would need it's own separate card. I don't love that we tried to get rid of the top and bottom save buttons by adding even more of them.

@mtias
Copy link
Member

mtias commented Dec 16, 2015

We should then put the blavatar interface to the left of those two fields and we create a sort of wysiwyg Site card editing interface.

Good idea. :)

Looping @adambbecker here as well.

@mikeshelton1503
Copy link
Contributor

👍 LGTM

This gave me an idea—I think the address should be the second item, right below the title. That mimics the Site card.

+1

@alternatekev
Copy link
Contributor Author

@mikeshelton1503 @rickybanister So Site Address would lose the bordering I've given it here, right? That's fine, I just wanted to confirm.

@rickybanister
Copy link

Well, I guess eventually. We don't have a blavatar interface yet. I'd say your change could get merged (if other's agree) and just keep the whole Site card style interface idea bubbling for later....

@alternatekev alternatekev added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR and removed [Status] In Progress labels Dec 17, 2015
removed redundant headers

tidies up the form a bit

removes the is-magic classname to show save buttons by default again

removed extraneous is-magic class from css
@alternatekev alternatekev force-pushed the try/move-site-address-to-site-profile branch from 25a28e9 to e5cbf7e Compare December 17, 2015 18:50
@mikeshelton1503
Copy link
Contributor

Well, I guess eventually. We don't have a blavatar interface yet. I'd say your change could get merged (if other's agree) and just keep the whole Site card style interface idea bubbling for later....

I agree. It doesn't need to happen right now but I like the idea of it for the future.

@alternatekev alternatekev removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Dec 17, 2015
{ this.languageOptions() }
{ this.holidaySnowOption() }
</form>
</Card>
<SectionHeader label={ this.translate( 'Address and visibility' ) }>
<SectionHeader label={ this.translate( 'Visibility' ) }>
<Button
Copy link
Member

Choose a reason for hiding this comment

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

This is an added bonus - because Jetpack sites do not require the custom address controls, so the 'Address and Visibility' label is a bit confusing there. Now it will be just 'Visibility' . Win win!

@roccotripaldi
Copy link
Member

Code looks solid. No errors and the custom address controls still work in the new layout. LGTM!

I agree that the blavatar could be handled in an other PR.

@alternatekev alternatekev removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 17, 2015
alternatekev added a commit that referenced this pull request Dec 17, 2015
…e-profile

Site Settings: Moves Site Address up to Site Profile card
@alternatekev alternatekev merged commit a009258 into master Dec 17, 2015
@alternatekev alternatekev deleted the try/move-site-address-to-site-profile branch December 17, 2015 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature Group] Emails & Domains Features related to email integrations and domain management. [Feature] Site Settings All other general site settings. [Type] Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants