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.

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

Feature/inline text editing #194

Merged
merged 15 commits into from
Nov 22, 2020

Conversation

Giwayume
Copy link
Contributor

@Giwayume Giwayume commented Nov 19, 2020

#185

Ok, this was quite an adventure. After lots of weird bugs, I'm at the point I can't find any more problems with it.

  • Full inline text editing implemented, toolbar attributes control the styling of selected text as opposed to acting only as a "starting point" for the text layer like most other tools.
  • Selection resizing was updated to not allow negative width/height, instead the layer is re-positioned. Changed string search on handle names to binary operations to speed up that logic.
  • Converts previous layer format to new format, due to possibility of every letter having unique style. Unfortunately, this won't look exactly the same due to the old code not calculating line height for the font correctly. Also possible slight differences in letter kerning due to rendering letters one-by-one.
  • Expanded the tool attributes config definition a bit since I needed better control over it.
  • Added "is_input" helper throughout code so this logic can be centralized and can lock keyboard controls in custom inputs (fixes bugs like using the arrow keys in color selection also won't affect selected tool keyboard controls).
  • Added Google "webfontloader" library for loading fonts, it gives better detection of when the font is ready for drawing on canvas than the pre-existing code (I believe the "preview" button in the old text editor dialog was a workaround for fonts not loading?)

Example of old (incorrect) line height calculation, overlapping lines:
image

Example of new line height calculation:
image

Once I add the leading control feature, the old to new format conversion script should be able to offset this so old files look the same (by setting a negative leading value).

@viliusle
Copy link
Owner

Amazing, so much work, and for user it looks really nice. There are many features, nice.

Before merge, there are few issues:

  • Do we really need "serve": "webpack-dev-server --mode development" ? We already have npm run server doing almost same.
  • There is basically no difference in top button styles between active button and inactive (tools > rectangle> fill).
  • Lets replace HTML on config.js to CSS class and keep this file clean and readable.
  • File > open > open test template gives error. Replace params.align.toLowerCase() to params.align.value.toLowerCase().
  • Update images/test-collection.json. You can skip it, i will update it later. @viliusle

I can fix last 3 issues. Please check "2" and comment about "1".

@Giwayume
Copy link
Contributor Author

Giwayume commented Nov 19, 2020

  • Do we really need "serve": "webpack-dev-server --mode development" ? We already have npm run server doing almost same.

No, I was messing around because I had some weird issue where it kept moving over to the browser every time I saved. I changed a browser setting to stop that, line can be removed.

  • There is basically no difference in top button styles between active button and inactive (tools > rectangle> fill).

I thought it looked ok with the green highlight & underline. It's more subtle than the current style for sure. I think it needs to be a little less in your face because it's not a big important aspect of the UI that you're trying to draw user's attention to.

I was also thinking earlier that the pattern for these buttons should change to show both options. Sometimes it's not clear what it means to deactivate the button.

  • Lets replace HTML on config.js to CSS class and keep this file clean and readable.

Yeah

@Giwayume
Copy link
Contributor Author

Giwayume commented Nov 19, 2020

I'll think about 2. Still have a lot of work left to do on the text editing anyways (future PR).

@Giwayume
Copy link
Contributor Author

This should make it more obvious

image

@viliusle viliusle merged commit b817035 into viliusle:master Nov 22, 2020
viliusle added a commit that referenced this pull request Nov 22, 2020
@viliusle
Copy link
Owner

All issues fixed, also File > open > open test template should show all text layer possibilities (including other layers of course)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants