WordPress.org

Make WordPress Core

Opened 12 months ago

Last modified 4 months ago

#41143 reviewing defect (bug)

Theme/plugin editing: if you don't select a function it just returns without message

Reported by: karmatosed Owned by: xkon
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch
Focuses: Cc:

Description

The drop down simply returns nothing, could we have a message to say something like 'please search for something'? It feels a little of a hitch in the flow otherwise.

https://cldup.com/B7kYDlxlUF.png

Attachments (7)

41143.diff (3.3 KB) - added by xkon 11 months ago.
Modified the buttons jQuery code to show / hide an error message.
41143_v2.diff (4.2 KB) - added by xkon 11 months ago.
Diff for suggestion with disabled button.
41143_v3.diff (3.4 KB) - added by xkon 11 months ago.
Disabled Button. Check code transfered to common.js
41143_4.diff (3.4 KB) - added by xkon 4 months ago.
Intergration with theme-plugin-editor.js
41143_5.diff (3.9 KB) - added by xkon 4 months ago.
Refactor
41143_5.gif (145.9 KB) - added by xkon 4 months ago.
Gif demonstration
41143_6.diff (3.7 KB) - added by xkon 4 months ago.
remove method

Download all attachments as: .zip

Change History (31)

#1 @karmatosed
12 months ago

  • Summary changed from Theme/plugin editing: if you don't have a function it doesn't encourage you to have one, just fails. to Theme/plugin editing: if you don't select a function it just returns without message

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


12 months ago

@xkon
11 months ago

Modified the buttons jQuery code to show / hide an error message.

#3 @xkon
11 months ago

Hello,

I created a simple patch for this by just adding a span, showing it / hiding it respectively.

It's for both theme-editor.php and plugin-editor.php

Screenshot:

https://dev.xkon.gr/forum_stuff/41143.jpg

If there are more suggestions let me know to come up with something else.

Best regards, Konstantinos

This ticket was mentioned in Slack in #core by xkon. View the logs.


11 months ago

#5 @xkon
11 months ago

  • Keywords has-patch added

#6 @swissspidy
11 months ago

What about disabling the button as long as nothing is selected?

#7 @xkon
11 months ago

Sure I could fix a version like that @swissspidy . To clarify it will be disabled by default and then accordingly changed if there's a selection .

I'll get on it after work. Question is there a js file that loads on admin globally it would be nicer to add a function there instead of inline. I have no clue first time contributing so I don't want to add things to wrong files ..

Best regards , Konatantinos

@xkon
11 months ago

Diff for suggestion with disabled button.

#8 @xkon
11 months ago

Hello again,

I've added 41143_v2.diff this is @swissspidy 's suggestion to have the button disabled. I think it works better than the error message. At least it makes more sense and keeps the UI clean.

Unfortunately I added the code within the page as I don't know if I should add it to the minified common.js myself or if somebody will transfer it there.

Keep me posted if you need it in another file so I can fix an update for it.

Best regards, Konstantinos

@xkon
11 months ago

Disabled Button. Check code transfered to common.js

#9 @xkon
11 months ago

After talking with @swissspidy ( he helped me understand the dev svn correctly, so thanks again :) ) I've added the code into common.js instead to be nice and clean for production.

Added: 41143_v3.diff

Best regards, Konstantinos

This ticket was mentioned in Slack in #core by xkon. View the logs.


11 months ago

This ticket was mentioned in Slack in #core-themes by xkon. View the logs.


11 months ago

#12 @SergeyBiryukov
11 months ago

  • Milestone changed from Awaiting Review to 4.9
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#13 @xkon
9 months ago

Just dropping by in case there's an update for this as well @karmatosed / @SergeyBiryukov , i reapplied the patch and it's still working by disabling the Look Up button

This ticket was mentioned in Slack in #design by xkon. View the logs.


9 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


8 months ago

#16 @westonruter
8 months ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 4.9 to 4.9.1
  • Owner changed from SergeyBiryukov to xkon

41143_v3.diff needs to be refactored to be integrated with theme-plugin-editor.js, which is new in this release.

#17 @johnbillion
7 months ago

  • Milestone changed from 4.9.1 to 5.0

@xkon
4 months ago

Intergration with theme-plugin-editor.js

#18 @xkon
4 months ago

  • Keywords has-patch added; needs-patch removed

@westonruter, I've added the code into theme-plugin-editor.js. It is working as supposed to but unfortunately I'm not that well versed to be honest and got lost a bit on how the 'refactoring' should work but at least I hope it's a start for someone to 'embed' it fully if this patch isn't viable.

If there are any pointers I'd gladly read up and re-work on it as every day is a new thing learned!

@xkon
4 months ago

Refactor

#19 @xkon
4 months ago

41143_5.diff adds into theme-plugin-editor.js the checks for Documentation: Lookup Button.

Looking it with a fresh and clear mind I think this is ready.

This ticket was mentioned in Slack in #design by xkon. View the logs.


4 months ago

@xkon
4 months ago

Gif demonstration

This ticket was mentioned in Slack in #design by boemedia. View the logs.


4 months ago

#22 @melchoyce
4 months ago

Looks good to me 👍

#23 @westonruter
4 months ago

Instead of a method name of docsLookup I think it would be better to have something more descriptive and “verby” like updateDocsLookUpButtonDisabledState. I'm not sure even a separate method is needed there, actually. The toggling of the disabled prop could be done right in the change handler.

@xkon
4 months ago

remove method

#24 @xkon
4 months ago

41143_6.diff removes the new method and does the check inside the change as @westonruter pointed out

Note: See TracTickets for help on using tickets.