feat(editable-title): make page title editable #553
Conversation
tilmx
changed the title
tilmx
changed the title
Palmaswell
changed the title
|
Looks good, just found an older bug that might be more obvious now.
|
| import { CopySize } from '../copy'; | ||
| import { getSpace, SpaceSize } from '../space'; | ||
|
|
||
| export enum EditState { |
There was a problem hiding this comment.
Should be EditableTitleState
There was a problem hiding this comment.
Please be aware that the store implementation was not changed in this feature branch.
I did the correction across the model types and the components.
| } | ||
|
|
||
| const StyledTitle = (props: StyledEditableTitleProps): JSX.Element => { | ||
| const Strong = styled.strong` |
There was a problem hiding this comment.
Remove StyledTitle and use Strong => StyledStrong directly.
You can add data-title via
const StyledStrong = styled.strong.attrs({ "data-title": true })``
There was a problem hiding this comment.
This component was inherited from the master branch and not changed during development.
I made the requested change, but not as proposed. Instead, I am setting the data-attr directly in the consumed Styled component.
<StyledTitle
data-title={true}
editable={props.focused}
fontSize={props.fontSize}
secondary={props.secondary}
>
{props.name}
</StyledTitle>Please note that I did try to use the attr constructor from Styled Components, nevertheless I was encountering problems with Typescript. Below you can see the error message.
Object literal may only specify known properties, and ''data-title'' does not exist in type 'Attrs<DetailedHTMLProps<HTMLAttributes<HTMLElement>, HTMLElement>,
I would say in this case that the request was fulfilled and the change should be accepted. Thanks!
| } | ||
|
|
||
| export interface EditableTitleProps { | ||
| fontSize?: CopySize; |
There was a problem hiding this comment.
Could fontSize be bound to secondary?
There was a problem hiding this comment.
That totally makes sense. Removed the fontSize prop
| focused: boolean; | ||
| name: string; | ||
| nameState: EditState; | ||
| secondary?: boolean; |
There was a problem hiding this comment.
Make this an enum, make it required
export EditableTitleOrder {
Primary,
Secondary
}
There was a problem hiding this comment.
Included the new enumerable to categorized the different EditableTitle component. Which I agree to be a good idea to give a much clearer idea of the component props intent :)
There was a problem hiding this comment.
I will take care now of abstracting the EditableTitleState.Editing and EditableTitleState.Editable store and include it to the container's component own state. I think this is the logical way to proceed in order to solve the problem mentioned below by @tilmx.
# Conflicts: # src/components/index.ts # src/components/page-tile/demo.tsx # src/components/page-tile/index.tsx # src/components/view-switch/index.tsx # src/container/chrome/chrome-container.tsx # src/container/page-list/page-tile-container.tsx
|
Heyhey! I just merged the current master into this branch. Now, as the title can be edited on the same view both in the chrome and the page pane, it doesn’t work that well any more. When I double-click the chrome page title, it automatically jumps into the page tile editing mode (and not into the "chrome editing mode"). I guess, we have to fix that as well... |
Palmaswell
force-pushed
the
feat/page-name
branch
from
b2d6435 to
4383050
Compare
|
Looks fine to me! Just to things:
|
feat(editable-title): make page title editable (meetalva#553)
No description provided.