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

Framework: removed last remaining instance of SectionHeaderButton #1468

Merged
merged 4 commits into from
Dec 11, 2015

Conversation

rickybanister
Copy link

I did several things:

  • Switched the 'Make Primary' domain button from the deprecated SectionHeaderButton to just a compact Button
  • Deleted the component finally
  • Updated the Devdocs
  • Updated the Readme

Before:
screen shot 2015-12-10 at 10 50 21 am

After:
screen shot 2015-12-10 at 10 49 50 am

cc @ebinnion @mtias @breezyskies

@rickybanister rickybanister added Framework [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 Components labels Dec 10, 2015
@rickybanister rickybanister self-assigned this Dec 10, 2015
compact>
{ this.translate( 'Manage' ) }
</Button>
<Button
Copy link
Member

Choose a reason for hiding this comment

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

Should we make one of these a primary one for the example?

@mtias
Copy link
Member

mtias commented Dec 10, 2015

Awesome, thanks for doing this.

@breezyskies
Copy link
Contributor

Make Primary button looks good. 👍

@rickybanister
Copy link
Author

I fixed some css and updated the devdocs to show a primary action:

screen shot 2015-12-10 at 11 18 49 am

@rickybanister
Copy link
Author

cc @shaunandrews as this affects #1391

<Button
compact
primary>
{ this.translate( 'Primary Action' ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

I could be wrong, but I don't think we need to translate the strings in examples. If we do keep the translation, we should probably at least provide some context. Maybe something like, this.translate( 'Primary Action', { context: 'Button display in documentation' } )?

cc @deBhal who might know whether we need to translate these strings.

Note: There's also another translated string in this file on line 33, and one more on line 45.

@ebinnion
Copy link
Contributor

I'm not sure what route to use to test this component: client/my-sites/upgrades/domain-management/edit/card/header/primary-domain-button.jsx. I'm glad to test if you can help me out with that.

Code-wise, this LGTM, except comment about not using translated strings in example components, but I'm not sure which way that should go.

@rickybanister
Copy link
Author

Please update the examples to fit the standard, I just saw translations in one file and not the other so I made it consistent. The contexts probably make sense too.

To test the domain button you have to visit a site that has a custom domain, then click on the domains sidebar item, then click on the non primary domain (i.e. example.wordpress.com)

The url would be something like:

https://wpcalypso.wordpress.com/domains/manage/example.wordpress.com/edit/example.com

@ebinnion ebinnion force-pushed the fix/no-more-sectionheaderbutton branch from f718dbb to dde16bb Compare December 11, 2015 17:31
ebinnion added a commit that referenced this pull request Dec 11, 2015
Framework: removed last remaining instance of SectionHeaderButton
@ebinnion ebinnion merged commit 1bfa47b into master Dec 11, 2015
@ebinnion ebinnion deleted the fix/no-more-sectionheaderbutton branch December 11, 2015 17:41
@scruffian scruffian removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 11, 2015
@ebinnion
Copy link
Contributor

I went ahead and removed the translated strings. I also was able to test that the domain management section header worked fine. 👍

@deBhal
Copy link
Contributor

deBhal commented Dec 14, 2015

Sorry for the late reply, I've been out of commission.

These strings won't get picked up or translated, so there's no concern over wasting translator's time, it's just a matter of communications with developers. (We scan for strings after we bundle up the code, so we skip docs and tests automagically. You can always make translate and check calypso-strings.js in the top level directory if you're not sure).

So it's just a question of whether it's better example code or not. I'd lean towards including the translate calls in general, unless you feel like they obscure the point of the example.

I feel like in this example, you gain a little, particularly with the label attribute where you get a bit of extra signalling that this is a user-facing string and not a control value.

I've opened PR #1536 because I feel bad about being away and missing the other PR, not to be pushy. If you feel like the example stands better without the changes, feel free to comment to that effect and close it.

@lancewillett lancewillett 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 Oct 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants