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

Enable adjustment of 'settings' values #120

Open
wants to merge 50 commits into
base: main
Choose a base branch
from

Conversation

chrishall0
Copy link
Contributor

@chrishall0 chrishall0 commented Dec 2, 2024

Description

Fixes #94

This PR adds the feature to enable adjustment of 'settings' values in the themer editor. Suitable individual components have been built for each setting value. This could do with a review from a UI/UX point of view.

Settings are accessed using a 'settings' button to the right of each section. Settings are available site wide and for each individual block.

image

Each component is currently wrapped in a panel for convenience as there's a lot of options available. We can adjust this to something more suitable if required.

Screen.Recording.2024-12-18.at.12.18.06.mov

I've added in repeating functionality where required (e.g. color palettes) and the option to edit or delete any created values.

image

Change Log

  • Adds in settings nav option
  • Adds in suitable components for each setting value

Steps to test

Open up themer and and click the new settings button available next to 'Site' and each block.
Test each component and check the values can be updated with the relevant data type and saved correctly.
Test the new options are now available in the post editor.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@chrishall0 chrishall0 changed the title feature/94-adjust-settings Enable adjustment of 'settings' values Dec 18, 2024
@chrishall0 chrishall0 requested a review from g-elwell January 2, 2025 13:32
Copy link
Member

@g-elwell g-elwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers for making those changes so quickly @chrishall0

Sorry if I'm drip feeding here, just been getting to grips with things and doing some more testing...

There are a few other values which I think should be reading/saving to .theme rather than .custom so they show up properly:

  • Gradient
  • Duotone
  • Shadow presets
  • Font Families
  • Font Sizes

In CodeView could we display the settings object when we navigate to the /settings panel? This will also help us see how the UI affects the settings object. It's a bit large so can be hard to see what you're editing, but better than nothing for now.

Editing spacing units seems as though it's not working correctly. Adding a value will overwrite an existing value and I'm unable to remove any of the existing values. Also, perhaps the UI should only allow specific preset values rather than free-text input.

Please can we change "Custom Settings" title to "Custom CSS" and rename component to SettingsCustomCSS to match?

Testing this with the code view in place using one of the core WordPress themes like twentytwentyfive or twentytwentyfour you can see certain values from theme.json which aren't supported. I don't necessarily think we need to support everything 100% but it could be worth documenting what is/isn't supported and we can add things if needed over time. Depends on how straightforward this is.

@chrishall0
Copy link
Contributor Author

Missing settings:

Spacing Settings

spacingScale

    "spacingScale": {
      "theme": {
        "operator": "*",
        "increment": 1.5,
        "steps": 7,
        "mediumStep": 1.5,
        "unit": "rem"
      }
    },

spacingSizes

"spacingSizes": {
      "theme": [
        {
          "name": "Tiny",
          "size": "10px",
          "slug": "20"
        },
        {
          "name": "X-Small",
          "size": "20px",
          "slug": "30"
        },
        {
          "name": "Small",
          "size": "30px",
          "slug": "40"
        },
        {
          "name": "Regular",
          "size": "clamp(30px, 5vw, 50px)",
          "slug": "50"
        },
        {
          "name": "Large",
          "size": "clamp(30px, 7vw, 70px)",
          "slug": "60"
        },
        {
          "name": "X-Large",
          "size": "clamp(50px, 7vw, 90px)",
          "slug": "70"
        },
        {
          "name": "XX-Large",
          "size": "clamp(70px, 10vw, 140px)",
          "slug": "80"
        }
      ]
    }

Dimension Settings
aspectRatios

"aspectRatios": {
      "theme": [
        {
          "name": "Example",
          "slug": "example",
          "ratio": "16/9"
        },
      ]

I will create tickets for these missing components

@chrishall0 chrishall0 requested a review from g-elwell January 9, 2025 11:54
Copy link
Member

@g-elwell g-elwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is shaping up really well @chrishall0 - I love it!

Editing colours

When editing a colour, can we also have the ability to update the name/slug?

Kapture.2025-01-09.at.14.35.42.mp4

The same goes for gradients and duotone - perhaps we could reuse the same component for both adding and editing?

Design changes

Please can we remove:

  • One layer of padding/border from the site settings area so the accordion is flush to the sides of the panel
  • The first item/title Site Settings as this duplicates the content above
  • The first heading inside each section, as this duplicates the accordion panel title

96070

Navigation

Would it be possible/straightforward for the navigation to the settings area to be a 'toggle' - in the sense when you click the button whilst the settings is open, it sends you back to the previous area you were editing?

Colour bugs/quirks

If you add a colour without touching the colour picker, it adds a new, blank, colour, and you can't click this to edit or delete it. Not sure if it's a bug when you try to click it, or if we should just add the colour as #000 if not interacted with, which should resolve the issue.

When a user is entering a slug, can we convert spaces to -?

@chrishall0 chrishall0 requested a review from g-elwell January 10, 2025 12:04
Copy link
Member

@g-elwell g-elwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chrishall0 this is looking great! Just want to tidy the UI up a little now:

  • Can we put some space between the add/delete buttons?
  • Can we add isDestructive prop to the delete button to make it red?
  • The buttons can probably just say 'add' and 'delete' - this means fewer unique strings, and we can use core strings which are already translated by referencing the core namespace, ie __("Add", "default")

13680

  • More of a gap between sub-sections would be a small change and should make things feel less 'cluttered'

13680

  • Maybe some margin below these headings too, it looks okay in places but tight in others. I think this is just due to some of the buttons having white padding around them as when you highlight one you can see it's actually flush to the heading above

33339

  • Perhaps we should put a border on these buttons as well, since when they wrap like in this screenshot the layout looks off?

18890

  • I noticed some unusual behaviour in the font family popup, there's no title, and no save button. The button opens the same popup again 🙃

83234

  • In the font sizes popup there's also no title and the save/delete buttons are white. Can we check all the modals to ensure they have the same layout and button styling? I haven't looked at the code yet but perhaps there's a way to make this consistent and easier on yourself by creating a reusable component? Whatever you think is best!

46498

@chrishall0 chrishall0 requested a review from g-elwell February 26, 2025 14:01
Copy link
Member

@g-elwell g-elwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the latest changes @chrishall0

The modals are still looking a bit inconsistent, please can we adjust them all to use the title prop, and ensure they have the correct add/edit labels?

  • Duotone modal - looks great ✅
  • Gradient modal - can this say Add New Gradient, Edit Gradient
  • Font Families modal - can this say Add New Font Family, Edit Font Family and use the title prop
  • Font Face modal (inside of font families modal) - Can this say Add New Font Face, Edit Font Face and use the title prop
  • Font Sizes modal - Can this say Add New Font Size, Edit Font Size and use the title prop
  • Shadows modal - Can this say Add New Shadow, Edit Shadow and use the title prop

Can we adjust the display of the font face inside of the font family modal? IMO this should:

  • Be above the save/delete buttons
  • Avoid stretching the modal width excessively (add a max-width, or stack vertically?) If there are many stacked vertically, we'd need to ensure this looks okay too
  • Show more info in the listing (see screenshot from my default config, all the items look the same so you can't tell what each one is for):
    38665

I hit a small bug when testing, by adding a gradient without selecting an angle, when you then click back in to view/edit this gradient, the component will crash. Can we add a check in the component to prevent crashing when the orientation doesn't exist? Also it might make sense to provide a default value of 0 when adding a new gradient, but I think the check would be a good idea regardless incase we load a theme.json with missing data.

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.

Enable adjustment of 'settings' values
2 participants