Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

feat: page overview #399

Merged
merged 16 commits into from
May 4, 2018
Merged

feat: page overview #399

merged 16 commits into from
May 4, 2018

Conversation

Palmaswell
Copy link
Member

Page Switch implementation

<ViewSwitch
fontSize={Size.M}
justify="center"
leftVisible={!!previousPage}
Copy link
Member

Choose a reason for hiding this comment

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

using Boolean(previousPage) would be a bit nicer

Copy link
Member Author

Choose a reason for hiding this comment

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

Done ✅

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@Palmaswell Palmaswell Apr 25, 2018

Choose a reason for hiding this comment

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

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

  1. 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.

  2. 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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done ✅


export interface ViewSwitchProps {
fontSize?: Size;
justify?: 'start' | 'center' | 'end' | 'stretch';
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 not very nice. You are just moving every css possibility in to the props.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

I think for now we should reduce them :)

Jumace
Jumace previously requested changes Apr 24, 2018
@@ -1,8 +1,11 @@
import Chrome from '../../lsg/patterns/chrome';
import { Size } from '../../lsg/patterns/copy';
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done ✅

<OverviewSwitchContainer />
<ViewSwitch
fontSize={Size.M}
justify="center"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done ✅

<ViewSwitch
fontSize={Size.M}
justify="center"
leftVisible={!!previousPage}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done ✅

fontSize={Size.M}
justify="center"
leftVisible={!!previousPage}
rightVisible={!!nextPage}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done ✅

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry, my bad. Done too :)

return (
<ViewSwitch
onLeftClick={() => store.togglePageOverview()}
onRightClick={() => null}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

}

const StyledViewSwitch = styled.div`
display: inline-flex;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

actually in this case you're right. ✅

@@ -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" />]],
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@Jumace
Copy link
Contributor

Jumace commented Apr 24, 2018

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
Copy link
Member

tilmx commented Apr 24, 2018

@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
Copy link
Member

@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 Palmaswell force-pushed the feat/page-switch branch 2 times, most recently from 0819e12 to 2d16109 Compare April 25, 2018 10:49
@Palmaswell
Copy link
Member Author

@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
Copy link
Member Author

@tilmx Regarding your comment.

  1. 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.

  2. 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
Copy link
Member

tilmx commented Apr 25, 2018

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
Copy link
Member

@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
Copy link
Member

tilmx commented May 3, 2018

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

@Palmaswell
Copy link
Member Author

Hi @tilmx,

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

@marionebl marionebl changed the title (feat): Page Switch feat: page switch May 3, 2018
@marionebl marionebl dismissed stale reviews from lkuechler and Jumace May 3, 2018 23:13

Adressed

@marionebl marionebl changed the title feat: page switch feat: page handling May 4, 2018
@marionebl marionebl changed the title feat: page handling feat: page overview May 4, 2018
@marionebl
Copy link
Contributor

marionebl commented May 4, 2018

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

Copy link
Member

@tilmx tilmx left a comment

Choose a reason for hiding this comment

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

Nice! Love it! <3

@marionebl marionebl merged commit 75f3c40 into master May 4, 2018
@marionebl marionebl deleted the feat/page-switch branch May 4, 2018 16:22
Palmaswell added a commit that referenced this pull request May 7, 2018
❤️ @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 pushed a commit that referenced this pull request May 7, 2018
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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants