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.

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

Themes: Sheet action bar #3333

Merged
merged 2 commits into from
Feb 22, 2016
Merged

Themes: Sheet action bar #3333

merged 2 commits into from
Feb 22, 2016

Conversation

ehg
Copy link
Member

@ehg ehg commented Feb 16, 2016

Implements part of #3160 and part of #3161

  • Looks like we'll need to sort out i18n now.
  • Styling needs to be more responsive(!)

Current state:
screen shot 2016-02-19 at 14 55 28
screen shot 2016-02-19 at 14 55 51

To test:

@ehg ehg added [Feature Group] Appearance & Themes Features related to the appearance of sites. [Status] In Progress labels Feb 16, 2016
@ehg ehg self-assigned this Feb 16, 2016
@ehg ehg added this to the Themes: Showcase M4-ThemeSheets milestone Feb 16, 2016
@ehg ehg mentioned this pull request Feb 16, 2016
Closed
4 tasks
@ehg
Copy link
Member Author

ehg commented Feb 17, 2016

@folletto when you get a chance, I could use some ideas with regard to the screenshot styling.

My latest approach is to float it in the action button container, so it never overlaps the buttons there. But it feels weird and hacky, see f4050fc's commit message for the problems.

@folletto
Copy link
Contributor

when you get a chance, I could use some ideas with regard to the screenshot styling.

Just noting what we chatted about: ideally we should write the markup as it was mobile, with the screenshot after the action bar, and then pad the action bar as needed to allow the screenshot to slide up. :)

@ehg ehg force-pushed the add/themes-sheet-action-bar branch from 2cabffe to a75bba0 Compare February 18, 2016 13:35
@ehg ehg mentioned this pull request Feb 18, 2016
Merged
blowery
blowery reviewed Feb 18, 2016
View reviewed changes
webpack.config.node.js
@@ -73,7 +73,10 @@ module.exports = {
},
plugins: [
// Require source-map-support at the top, so we get source maps for the bundle
new webpack.BannerPlugin( 'require( "source-map-support" ).install();', { raw: true, entryOnly: false } )
new webpack.BannerPlugin( 'require( "source-map-support" ).install();', { raw: true, entryOnly: false } ),
new webpack.NormalModuleReplacementPlugin( /^analytics$/, 'lodash/utility/noop' ), // noop analytics module until we make it ssr-ready
Copy link
Contributor

Choose a reason for hiding this comment

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

lodash 4 compat warning on this path

@ehg ehg force-pushed the add/themes-sheet-action-bar branch 3 times, most recently from c9bd8ee to f059c5e Compare February 22, 2016 12:21
@ehg ehg mentioned this pull request Feb 22, 2016
Closed
3 tasks
@ehg ehg added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Feb 22, 2016
@seear seear force-pushed the add/themes-sheet-action-bar branch from 354353d to e4d1fc3 Compare February 22, 2016 14:03
@seear
Copy link
Contributor

seear commented Feb 22, 2016

View the details of a theme via a Popover menu
Check theme sheet is displayed OK, and navigating backwards and forwards works

The scroll position is preserved, cutting off the top of the details sheet.

seear
seear reviewed Feb 22, 2016
View reviewed changes
client/my-sites/themes/sheet.jsx
<div className="themes__sheet-content">
<SectionNav className="themes__sheet-section-nav" selectedText={ filterStrings[section] }>
<NavTabs label="Details" >
<NavItem path={ `/themes/${ this.props.tSemeSlug }/details` } selected={ section === 'details' } >{ i18n.translate( 'Details' ) }</NavItem>
Copy link
Contributor

Choose a reason for hiding this comment

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

tSemeSlug

Copy link
Member Author

Choose a reason for hiding this comment

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

Hah :) Fixed.

@ehg
Copy link
Member Author

ehg commented Feb 22, 2016

The scroll position is preserved, cutting off the top of the details sheet.

Ah, I'm not sure there's an easy way of solving this. With InfiniteList we had https://github.com/Automattic/wp-calypso/tree/master/client/lib/infinite-list — but I'm thinking this should be solved at the router level.

seear
seear reviewed Feb 22, 2016
View reviewed changes
client/my-sites/themes/sheet.jsx
render() {
let actionTitle;
if ( this.props.isLoggedIn && this.props.theme.active ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can/should all this logic be shared with onPrimaryClick() somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think so. I held off on refactoring this, because it reminded me of the getButtonOptions stuff. Perhaps we could reuse some of that code?

Copy link
Contributor

Choose a reason for hiding this comment

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

it reminded me of the getButtonOptions stuff

Good point. Fine to tackle it later, anyway.

@seear
Copy link
Contributor

seear commented Feb 22, 2016

Looks good and we can iterate once merged 👍

ehg added 2 commits February 22, 2016 14:36
@ehg
Themes: Sheet action bar & content box
64511c3 
First iteration of some basic structural elements/CSS. We're not using
actual theme data yet. Here we:
- Ensure dependencies are ssr-ready
- Use HeaderCake & Button for the action bar
- Add some semi-working styling/markup
- Add activate/customize/choose functionality to the action bar primary
  button
- Add dummy content section + working SectionNav logic, so we can iterate on the CSS
- Initialise i18n on the server routes
@ehg
Themes: Enable /themes/<x> on wpcalypso
6ac7e8d
@ehg ehg force-pushed the add/themes-sheet-action-bar branch from 6d39e88 to 6ac7e8d Compare February 22, 2016 14:42
ehg added a commit that referenced this pull request Feb 22, 2016
@ehg
Merge pull request #3333 from Automattic/add/themes-sheet-action-bar
9ae0b11 
Themes: Sheet action bar
@ehg ehg merged commit 9ae0b11 into master Feb 22, 2016
@ehg ehg deleted the add/themes-sheet-action-bar branch February 22, 2016 14:53
@ehg ehg removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 22, 2016
This was referenced Feb 20, 2022
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ehg ehg

Labels
[Feature Group] Appearance & Themes Features related to the appearance of sites.
Projects
None yet
Milestone
Themes: Showcase M4-ThemeSheets
Development

Successfully merging this pull request may close these issues.
5 participants
@ehg @folletto @seear @blowery @ockham