feat: page overview #399

Palmaswell · 2018-04-24T14:06:58Z

Page Switch implementation

lkuechler · 2018-04-24T14:32:43Z

src/component/chrome/chrome-container.tsx

+				<ViewSwitch
+					fontSize={Size.M}
+					justify="center"
+					leftVisible={!!previousPage}


using Boolean(previousPage) would be a bit nicer

lkuechler · 2018-04-24T14:36:19Z

src/lsg/patterns/view-switch/index.tsx

+	fontSize?: Size;
+	justify?: 'start' | 'center' | 'end' | 'stretch';
+	leftVisible: boolean;
+	onLeftClick: React.MouseEventHandler<SVGElement>;


please name all event handler handle.... This way they are a lot easier to differentiate from the native react event handlers

What I've so far understood is that the convention is:

onClick: for defining props names. For the elements just receiving the props or passing them. So on is describing the actual event this will be tied to.

handleClick: for the component that is actually operating or acting on the click event. is handle describing what will be called when that event fires.

What is your opinion about that?

Currently we only use on when we use a pass something to a native react event prop.
This has the benefit that you can immediately see which event handlers are custom ones and could have a different response than you would assume.

lkuechler · 2018-04-24T14:41:11Z

src/component/chrome/chrome-container.tsx

+					justify="center"
+					leftVisible={!!previousPage}
+					rightVisible={!!nextPage}
+					onLeftClick={() => (previousPage ? store.openPage(previousPage.getId()) : undefined)}


onLeftClick is not optional. It should not be valid to pass undefined as a value here.
Maybe just always pass the function. I will not be triggered because the buttons are not visible if not needed

lkuechler · 2018-04-24T14:44:18Z

src/lsg/patterns/view-switch/index.tsx

+
+export interface ViewSwitchProps {
+	fontSize?: Size;
+	justify?: 'start' | 'center' | 'end' | 'stretch';


This is not very nice. You are just moving every css possibility in to the props.

Well, that is right. When more items come in chrome we will be going to need them :).
Would you like me to reduce them to the current necessary?

I think for now we should reduce them :)

Jumace · 2018-04-24T15:59:03Z

src/component/chrome/chrome-container.tsx

@@ -1,8 +1,11 @@
 import Chrome from '../../lsg/patterns/chrome';
+import { Size } from '../../lsg/patterns/copy';


import { Size } from '../../lsg/patterns/copy'; => import { Size as FontSize } from '../../lsg/patterns/copy';
With that it is more clear which size is meant. In the copy component it's kind of obvious but not here in the chrome-container

Jumace · 2018-04-24T16:04:02Z

src/component/chrome/chrome-container.tsx

+				<OverviewSwitchContainer />
+				<ViewSwitch
+					fontSize={Size.M}
+					justify="center"


please create a type for justify in src/lsg/patterns/view-switch/index.tsx and export it so it can be used here

Jumace · 2018-04-24T16:06:22Z

src/component/chrome/chrome-container.tsx

+				<ViewSwitch
+					fontSize={Size.M}
+					justify="center"
+					leftVisible={!!previousPage}


please refactror this too to use !! but Boolean(). The !! is pretty easy to oversee but Boolean() is easy to see. Afaik that is also something that was agreed on for this project. Correct me if I'm wrong

Jumace · 2018-04-24T16:06:31Z

src/component/chrome/chrome-container.tsx

+					fontSize={Size.M}
+					justify="center"
+					leftVisible={!!previousPage}
+					rightVisible={!!nextPage}


please refactror this too to use !! but Boolean(). The !! is pretty easy to oversee but Boolean() is easy to see. Afaik that is also something that was agreed on for this project. Correct me if I'm wrong

Jumace · 2018-04-24T16:14:36Z

src/component/chrome/overview-switch-container.tsx

@@ -0,0 +1,32 @@
+// import * as MobX from 'mobx';


if you don't need this one please delete the line

sorry, my bad. Done too :)

Jumace · 2018-04-24T16:19:54Z

src/component/chrome/overview-switch-container.tsx

+		return (
+			<ViewSwitch
+				onLeftClick={() => store.togglePageOverview()}
+				onRightClick={() => null}


if you don't need that function always, please consider to make it optional

Jumace · 2018-04-24T16:36:50Z

src/lsg/patterns/chrome/demo.tsx

@@ -0,0 +1,30 @@
+import { Size } from '../copy';


import { Size } from '../../lsg/patterns/copy'; => import { Size as FontSize } from '../../lsg/patterns/copy';
With that it is more clear which size is meant. In the copy component it's kind of obvious but not here in the chrome-container

Jumace · 2018-04-24T16:49:18Z

src/lsg/patterns/view-switch/index.tsx

+}
+
+const StyledViewSwitch = styled.div`
+	display: inline-flex;


doesn't have to be inline-flex, flex only is enough

actually in this case you're right. ✅

Jumace · 2018-04-24T16:53:15Z

src/lsg/patterns/icons/index.tsx

@@ -41,6 +42,7 @@ const icons: { readonly [key: string]: JSX.Element[][] | JSX.Element[] } = {
 		[<path key="arrow" d="M17.5 12l-8.486 8.485L7.6 19.071 14.671 12 7.6 4.929l1.414-1.414z" />]
 	],
 	[IconName.ArrowFill]: [[<path key="arrowFill" d="M8 4l8 8-8 8z" />]],


please refactor this one the ArrowFillRightwith that it will be easier to use the correct one

Jumace · 2018-04-24T16:56:34Z

src/lsg/patterns/view-switch/index.tsx

@@ -0,0 +1,80 @@
+import { colors } from '../colors';
+import { Size } from '../copy';


import { Size } from '../../lsg/patterns/copy'; => import { Size as FontSize } from '../../lsg/patterns/copy';
With that it is more clear which size is meant. In the copy component it's kind of obvious but not here in the chrome-container

Jumace · 2018-04-24T17:02:28Z

When I tried the feature the first thing I tried was to click on Meet Alva Overview so the "label" but nothing happened. In my opinion the label should be clickable too. @markusoelhafen maybe that is more a question for you.

tilmx · 2018-04-24T18:56:25Z

@Palmaswell Out of design perspective: Looks good to me – I only came across two thingies:

When I double-click on the page name to rename it, it opens the page. This should not happen – when double-clicking on the name, I expect to go into the renaming-mode.
The page switch in the top chrome should be hidden when you are in the page overview mode. So I mean the "< Page Title >" on the top.

lkuechler · 2018-04-25T07:23:39Z

@tilmx I talkt with @Palmaswell yesterday about the hiding of the page switch on the page overview.
We decided that it could also make sense to just keep it there but also highlight the currently selected page in the page overview.
This way we also don't have a chrome where element appear and reappear all the time ;)

Whats your opinion on that suggestion?

Palmaswell · 2018-04-25T10:51:34Z

@Jumace , @markusoelhafen , @tilmx

Regarding @Jumace comment regarding the clickable label. I think he is right, but if we decide to do so please make a new ticket for this feature. This is a behavior that is already in Master

Palmaswell · 2018-04-25T11:09:54Z

@tilmx Regarding your comment.

We decided that the page item should behave the same way files behave in Finder. In Finder when you double click on the file name, you actually open the document.
Please reply to @lkuechler about his suggestion, which I think its a very good idea. But we need your feedback in order to implement the functionality.

tilmx · 2018-04-25T11:23:12Z

Hey @Palmaswell, @Jumace and @lkuechler

Overview Link
as shown in the issue #254 the entire element ("< Meet Alva Overview") should actually have a hover state and should be clickable. Or do you have any concerns about this UI pattern?

Double Click on Page Name
The cursor: text is misleading then. It suggests that you rename the element when clicking or double-clicking on it. We’ll also have renaming actions in the element pane (and some other areas) – and there we can only use the double-click to rename it. I’d go for that double-clicking on the element starts the edit mode to make it consistent through the whole UI. Or could we integrate both? So both double click and the 2. "slow" click enters edit mode? Sketch does it that way.

Page Switch in Chrome
To my mind there is not a real reason to switch the pages when can not see any effect – as described in the issue #176 – could we show the project name there instead?

<3
Tilman

lkuechler · 2018-04-26T10:07:58Z

@tilmx Page Switch in Chrome:
There could be something that changes when switching pages on the page-overview page. The selected page on the page-overview could change when the currently active page is changed.

tilmx · 2018-05-03T05:50:41Z

Hey @Palmaswell! What’s the status here? Can we finish this PR this week?
Thanks, Tilman

Palmaswell · 2018-05-03T07:06:27Z

Hi @tilmx,

I am on holidays until Monday. I guess it must wait until then.

Adressed

marionebl · 2018-05-04T09:21:42Z

Page Switch in Chrome
To my mind there is not a real reason to switch the pages when can not see any effect – as
described in the issue #176 – could we show the project name there instead?

Addressed

The cursor: text is misleading then. It suggests that you rename the element when clicking or double-clicking on it. We’ll also have renaming actions in the element pane (and some other areas) – and there we can only use the double-click to rename it. I’d go for that double-clicking on the element starts the edit mode to make it consistent through the whole UI. Or could we integrate both? So both double click and the 2. "slow" click enters edit mode? Sketch does it that way.

Addressed

tilmx

Nice! Love it! <3

@Palmaswell

❤️ @Palmaswell * feat: add page overview * chore: update lock file * fix: update usage of arrow icons * fix: be consistent about event listener names * fix: consistently indicate texts can be edited * fix: truncate too long page titles * fix: adjust project title styling and page inset * fix: visually align chrome title * fix: adapt styling of view-switch * fix: ensure page list title clicks enable edit mode * feat: show view button only page detail view * fix: visually align chrome title * feat: wrap the page list for narrow viewports * fix: manage preview resizing lifecycle correctly

@Palmaswell

fix: avoid jumping heights via text-overflow: ellipsis feat: page overview (#399) :heart: @Palmaswell * feat: add page overview * chore: update lock file * fix: update usage of arrow icons * fix: be consistent about event listener names * fix: consistently indicate texts can be edited * fix: truncate too long page titles * fix: adjust project title styling and page inset * fix: visually align chrome title * fix: adapt styling of view-switch * fix: ensure page list title clicks enable edit mode * feat: show view button only page detail view * fix: visually align chrome title * feat: wrap the page list for narrow viewports * fix: manage preview resizing lifecycle correctly feat: add floating-button component (#425) * feat: add floating-button component * fix: add plus icon to icon library feat: allow users to add new pages (#426) fix(highlight): include blur behaviour feat(preview-renderer): Rebase master status

Palmaswell requested a review from Jumace April 24, 2018 14:07

Palmaswell force-pushed the feat/page-switch branch from f230067 to d24b2b7 Compare April 24, 2018 14:33

lkuechler previously requested changes Apr 24, 2018

View reviewed changes

Jumace previously requested changes Apr 24, 2018

View reviewed changes

Palmaswell force-pushed the feat/page-switch branch 2 times, most recently from 0819e12 to 2d16109 Compare April 25, 2018 10:49

marionebl changed the title ~~(feat): Page Switch~~ feat: page switch May 3, 2018

feat: add page overview

7edd498

marionebl force-pushed the feat/page-switch branch from 9ae3a6c to 7edd498 Compare May 4, 2018 08:08

marionebl and others added 4 commits May 4, 2018 10:18

Merge remote-tracking branch 'origin/master' into feat/page-switch

cb621e7

chore: update lock file

79cb041

fix: update usage of arrow icons

1753c3d

Merge remote-tracking branch 'origin/master' into feat/page-switch

c7116f1

marionebl changed the title ~~feat: page switch~~ feat: page handling May 4, 2018

marionebl changed the title ~~feat: page handling~~ feat: page overview May 4, 2018

marionebl added 3 commits May 4, 2018 12:42

fix: be consistent about event listener names

15e8f1d

fix: consistently indicate texts can be edited

113519d

fix: truncate too long page titles

f466689

tilmx and others added 8 commits May 4, 2018 14:18

fix: adjust project title styling and page inset

b3802e7

fix: visually align chrome title

2a6b00d

fix: adapt styling of view-switch

a312f82

fix: ensure page list title clicks enable edit mode

6746fcc

feat: show view button only page detail view

4342ea8

fix: visually align chrome title

28e898d

feat: wrap the page list for narrow viewports

c1124d7

fix: manage preview resizing lifecycle correctly

2958ff3

tilmx approved these changes May 4, 2018

View reviewed changes

marionebl merged commit 75f3c40 into master May 4, 2018

marionebl deleted the feat/page-switch branch May 4, 2018 16:22

		@@ -1,8 +1,11 @@
		import Chrome from '../../lsg/patterns/chrome';
		import { Size } from '../../lsg/patterns/copy';

		@@ -0,0 +1,80 @@
		import { colors } from '../colors';
		import { Size } from '../copy';

feat: page overview #399

feat: page overview #399

Conversation

Palmaswell commented Apr 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Palmaswell Apr 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jumace commented Apr 24, 2018

tilmx commented Apr 24, 2018 • edited

lkuechler commented Apr 25, 2018

Palmaswell commented Apr 25, 2018

Palmaswell commented Apr 25, 2018

tilmx commented Apr 25, 2018 • edited

lkuechler commented Apr 26, 2018

tilmx commented May 3, 2018

Palmaswell commented May 3, 2018

marionebl commented May 4, 2018 • edited

tilmx left a comment

Choose a reason for hiding this comment

Palmaswell Apr 25, 2018 •

edited

tilmx commented Apr 24, 2018 •

edited

tilmx commented Apr 25, 2018 •

edited

marionebl commented May 4, 2018 •

edited