Skip to content
This repository has been archived by the owner on Mar 21, 2018. It is now read-only.

Implement page summaries (#747) #821

Closed
wants to merge 17 commits into from

Conversation

victorporof
Copy link
Contributor

@victorporof victorporof commented Jul 14, 2016

Fixes #747

Looking to start landing this incrementally. Here's a first couple of commits.
A few more coming soon after I finish cleaning up.
@bgrins

@jsantell jsantell removed the review? label Jul 14, 2016
@victorporof victorporof changed the title Implement page summaries Implement page summaries (#747) Jul 14, 2016
@victorporof victorporof force-pushed the page-summaries branch 2 times, most recently from cca309e to 3382e1b Compare July 14, 2016 14:23
@victorporof victorporof force-pushed the page-summaries branch 2 times, most recently from efb3c42 to 39fc72d Compare July 14, 2016 15:06
@bgrins
Copy link
Member

bgrins commented Jul 14, 2016

Pulled this down. A UI note I noticed first is the extra spacing and separators between the active tab and inactive: extra separators and spacing

@bgrins
Copy link
Member

bgrins commented Jul 14, 2016

Is this ultimately planning to land in a branch for a particular experiment or on master?

@bgrins
Copy link
Member

bgrins commented Jul 15, 2016

Is this ultimately planning to land in a branch for a particular experiment or on master?

Discussed this in the meeting today, and sounds like it's fine to land this on master with the knowledge that we might want to ultimately hide the 'overview' button or other UI elements while testing other features. IOW we can consider the top bar / nav bar changes present here the new 'default' UI. @phlsa please correct me if that's wrong.

@victorporof victorporof force-pushed the page-summaries branch 4 times, most recently from ac5dc87 to 53b2882 Compare July 18, 2016 08:16
@victorporof victorporof force-pushed the page-summaries branch 4 times, most recently from 9867d25 to cfb47f1 Compare July 18, 2016 09:29
@victorporof
Copy link
Contributor Author

Landed some of the changes in 9770df4

Further commits will follow in this PR.

@victorporof victorporof force-pushed the page-summaries branch 7 times, most recently from feebc67 to b4f00d7 Compare July 19, 2016 06:44
Victor Porof added 5 commits July 20, 2016 19:47
Signed-off-by: Victor Porof <vporof@mozilla.com>
…ies are visible

Signed-off-by: Victor Porof <vporof@mozilla.com>
Signed-off-by: Victor Porof <vporof@mozilla.com>
Signed-off-by: Victor Porof <vporof@mozilla.com>
Signed-off-by: Victor Porof <vporof@mozilla.com>
@victorporof
Copy link
Contributor Author

Rebased.

@@ -55,6 +55,17 @@ export default function uiState(state = initialState, action) {
case types.SET_URL_INPUT_AUTOCOMPLETE_INDEX:
return state.set('focusedResultIndex', action.payload.index);

case types.TOGGLE_PAGE_SUMMARIES: {
Copy link
Member

Choose a reason for hiding this comment

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

We've decided in the past to not use TOGGLE_* as an action, instead preferring SET_* so that the action log / frontend is more explicit about what it's requesting. See something like SET_URL_INPUT_VISIBLE as an example

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed on this

@bgrins
Copy link
Member

bgrins commented Jul 20, 2016

Should there be a separator here?

screenshot 2016-07-20 10 07 08

@bgrins
Copy link
Member

bgrins commented Jul 20, 2016

extra separator when the last tab is selected tab: screenshot 2016-07-20 11 40 15

@jsantell
Copy link
Contributor

This looks overall good to me -- some visual bugs that Brian pointed out, but easier to fix it with it landed, and can begin tackling the harder issue of custom cards

@jsantell jsantell added review+ and removed review? labels Jul 20, 2016
@jsantell jsantell assigned jsantell and unassigned bgrins Jul 20, 2016
@jsantell
Copy link
Contributor

@bgrins passed this over to me -- after looking through the code, I don't have really any comments other than the naming of the action and maybe the granularity of some of the views, like Thumbnail and FittedImage, especially as we don't have other consumers of those (yet -- maybe you're anticipating some things). Or maybe just a preference. Either way, some of the lower level components could benefit from brief comment descriptions on how/when to use them (if I'm building something else looking at components, I should be able to quickly know when I'd use thumbnail vs fitted image).

Looks great! Once landed we can file follow up bugs and tackle the custom cards.

@victorporof
Copy link
Contributor Author

@bgrins
Mockups seem to disagree with you: https://cloud.githubusercontent.com/assets/514315/16498113/82b31796-3efa-11e6-8bba-f112192c28b2.png

But since I'm not really sure, I'll land as is and we can get @phlsa's feedback later.

@victorporof
Copy link
Contributor Author

@jsantell FittedImage and Thumbnail both have comments that describe what they're intended for.

Granular components are always a good idea, and incredibly useful once we properly start using shouldComponentUpdate. I'd suggest moving forward as we decided in Vancouver: very granular components for everything: better separation of concerns, better performance, better composability, cleaner code.

@phlsa
Copy link

phlsa commented Jul 21, 2016

I'm not entirely sure what you are referring to @victorporof.
If it's about the separators, the idea is that they disappear when they would be next to a selected tab, much like they do in Firefox today.

Victor Porof added 8 commits July 21, 2016 17:02
…ying on a toggle mechanism

Signed-off-by: Victor Porof <vporof@mozilla.com>
Signed-off-by: Victor Porof <vporof@mozilla.com>
Signed-off-by: Victor Porof <vporof@mozilla.com>
Signed-off-by: Victor Porof <vporof@mozilla.com>
Signed-off-by: Victor Porof <vporof@mozilla.com>
Signed-off-by: Victor Porof <vporof@mozilla.com>
Signed-off-by: Victor Porof <vporof@mozilla.com>
Signed-off-by: Victor Porof <vporof@mozilla.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants