tomusborne / generateblocks

GenerateBlocks is a small collection of lightweight WordPress blocks that can accomplish nearly anything.
https://generateblocks.com
188 stars 18 forks source link

Conditionally remove google toggle #1239

Open dgwyer opened 1 month ago

dgwyer commented 1 month ago

@tomusborne I was seeing if there was an easy way to conditionally remove the 'Use Google Fonts API' toggle from the block editor inspector panel if a system fonts, or font library font was selected.

I was hoping adding an id to each group of options in the <AdvancedSelect /> dropdown would allow us to target a specific group when an option was changed but this doesn't seem to be available in the onChange callback.

Not sure this is possible without some refactoring of the component?

tomusborne commented 1 month ago

Couldn't we just not render it if isFontLibraryItem is true as set in #1234?

dgwyer commented 1 month ago

I'm not sure that gives us enough information to make a determination. Sure, it tells us if the currently selected font is found in the font-library array (in typographyOptions), but there may also potentially be a match for the selected font in the system-fonts and/or google-fonts array.

Ideally we need a way to determine the source of the selected font as it could originate from any of the three categories system, google, or library.

tomusborne commented 1 month ago

I would argue that the "Use Google Fonts API" toggle is only useful if the font is found in the Google fonts array and not the font library. I don't see why something would willingly use the API if they've added the font locally, at least not on purpose.

dgwyer commented 1 month ago

So if it's found in the font library array (or system font array) array should we hide the toggle do you think? Would be simpler.

If so, I need to update the latest commit as I misread your previous comment. At the moment the Google API toggle is shown if the current font is present in the Google array regardless of whether it is also in the font library array. Should be an easy update.

tomusborne commented 1 month ago

Yea, I think if the font exists in the library, we just hide the toggle (and turn it off if it's on).

dgwyer commented 1 month ago

So the Google API toggle is now hidden if the selected font is from system or the library. If the toggle is hidden it is also disabled to prevent the case where it has been hidden but the value is still set to active.

Also, while poking around the code and testing this PR I'm seeing a few isolated cases of this type of error:

image

It can be difficult to track down as there isn't much information in the console error apart from it's somewhere in the inspector controls! I tracked this one down to TextControl in the UnitControl component. It seems this happens if the input value happens to be assigned to undefined/null etc.

I just updated this to always have a fallback value if undefined, which seems to fix the issue.

<TextControl
    type="text"
    value={ numericValue ? numericValue : '' }
    ...
/>
tomusborne commented 1 month ago

@dgwyer Just to confirm, this looks like a replacement/improvement over https://github.com/tomusborne/generateblocks/pull/1234, correct?

dgwyer commented 1 month ago

@dgwyer Just to confirm, this looks like a replacement/improvement over #1234, correct?

@tomusborne That's right. I think we can close that for now? We can add any specific utilitiy functions as we go, as needed.