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

Auto-completion in HTML panel #105

Closed
wants to merge 10 commits into from

Conversation

simonlindholm
Copy link
Member

Assorted fixes for issues 3931, 3700, 6854, 4458, 6814. (Still wanted: auto-completion for "class" attribute; only including the SVG tags/attributes supported by Firefox.)

Anyone wanna try this out and see if anything feels buggy? In particular, behavior of the tab key within the "style" attribute is weird and needs some discussion. (And I guess Enter should be the same as Tab.)

Code comments are of course also welcome.

@fflorent
Copy link
Member

Awesome!

I guess you should put yourself as the owner of these issues and reference that pull request on each of them.

Florent

@simonlindholm
Copy link
Member Author

I guess you should put yourself as the owner of these issues and reference that pull request on each of them.

Eh, I'm lazy. It doesn't reach anyone who doesn't see this PR already.

Anyway, so most of this I'm reasonably confident in and doesn't need much testing. I'm looking for feedback on:

  • general remarks on how it feels to use HTML attribute auto-completion
  • how to handle inside "style" attributes.
  • (code)

Expanding on the latter item, here's STR:

  • checkout the branch
  • in the HTML panel, create an attribute "style" for any element
  • type "c" (will auto-complete to "color"), press tab. What should happen? (currently it fills in "color: ")
  • type "b" (will auto-complete to "black"), press tab. What should happen? (currently it fills in "color: black", w/o semicolons*)
  • clear field, type "c:". What should happen? (currently auto-corrects to "color:")

etc. (just play around with style attributes and maybe other attributes too - the interesting thing is how it feels to use, what feels weird, what's missing etc.)

* motivation: if we include a single semicolon users will end up with things like "color: black;background: red" which looks weird; if we include "; " then we get trailing spaces...

(cc @SebastianZ @janodvarko)

@simonlindholm
Copy link
Member Author

Latest idea to handle tab in "style" attribute: add ": " or "; ", and then remove trailing spaces when editing ends.

@SebastianZ
Copy link
Member

Sorry for the late reply on this.
Wow! You put a lot of efforts into this.
Sorry for the late reply. I finally tried it out now and it's working almost exactly as I would expect it.
Only two small remarks so far:

  • Already existing attributes are still appearing in the auto-completion suggestions. This happens for both, the attribute completion as well as the style completion.
  • The only little thing I saw was that when typing a colon within the property name or a semicolon within the value the space after it is not added. IMO the behavior should be the same as when pressing Tab.

To answer your questions:

  • type "c" (will auto-complete to "color"), press tab. What should happen? (currently it fills in "color: ")

"color: " is fine.

type "b" (will auto-complete to "black"), press tab. What should happen? (currently it fills in "color: black", w/o semicolons*)

Should be completed to "color: black; " (as it's done now).

clear field, type "c:". What should happen? (currently auto-corrects to "color:")

Should be completed to "color: " (with space).

I didn't check the code related to the changes yet. Won't have enough time the next days either to do that. So someone else should review it.

Sebastian

@simonlindholm
Copy link
Member Author

Already existing attributes are still appearing in the auto-completion suggestions.

This is a good point. I found the related issue 4457, and I like the suggestion to automatically jump to the right attribute value after typing an attribute name and tab (kindof like Firefox's "switch to tab" in the location bar). But yes, in the mean time we should absolutely remove already set attribute names from auto-completion.

This happens for both, the attribute completion as well as the style completion.

Actually it happens even for the CSS and Style panels. But this feels like a much less often encountered problem, and it's a bit trickier to get right. Combine that with that the error behavior is much better than for attributes I'm inclined to leave this as it is.

The only little thing I saw was that when typing a colon within the property name or a semicolon within the value the space after it is not added.

Do we really want that? I would expect the result of typing color: red to be exactly color: red, not color:  red.

@SebastianZ
Copy link
Member

Already existing attributes are still appearing in the auto-completion suggestions.

This is a good point. I found the related issue 4457

Yes, I also had that issue in mind, but it can be done later.

This happens for both, the attribute completion as well as the style completion.

Actually it happens even for the CSS and Style panels. But this feels like a much less often encountered problem, and it's a bit trickier to get right. Combine that with that the error behavior is much better than for attributes I'm inclined to leave this as it is.

Well, if the list of properties is long, it might happen that you define a property a second time. Anyway, this can be done later, too.

The only little thing I saw was that when typing a colon within the property name or a semicolon within the value the space after it is not added.

Do we really want that? I would expect the result of typing color: red to be exactly color: red, not color: red.

The second space is not added automatically. Again, for consistency I'd say Tab and colon/semicolon should have the same behavior (like they have inside the CSS and Style panel).
What we could do is to differenciate between completely written property names and partly written ones. I.e. add a space when it's not complete, but don't add it when it is written completely.

Sebastian

@simonlindholm
Copy link
Member Author

What we could do is to differenciate between completely written property names and partly written ones. I.e. add a space when it's not complete, but don't add it when it is written completely.

I added this - it probably works well enough. Do you want/have time to review this at some point?

@SebastianZ
Copy link
Member

What we could do is to differenciate between completely written property names and partly written ones. I.e. add a space when it's not complete, but don't add it when it is written completely.

I added this - it probably works well enough. Do you want/have time to review this at some point?

Works fine for me. Thanks!

Found one little other thing: Pressing Tab when you've completely written the property name should still complete the ": " and stay within the "style" attribute value. Currently you're jumping to the next attribute.

Sebastian

@simonlindholm
Copy link
Member Author

Sure, I can fix that. But for property values we should still jump to the next attribute, right?

@SebastianZ
Copy link
Member

Right.

@simonlindholm
Copy link
Member Author

done

@simonlindholm
Copy link
Member Author

Rebased, and slightly refactored. I guess I should just merge this (after adding tests).

@simonlindholm
Copy link
Member Author

Thank you for the review, Sebastian!

@SebastianZ
Copy link
Member

No problem. Sorry that I didn't do that earlier!

Sebastian

@simonlindholm
Copy link
Member Author

Comments addressed except for the line length thing.

@simonlindholm
Copy link
Member Author

Merged in 2763f22.

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

4 participants