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

Update field CSS #104

Closed
wants to merge 4 commits into from
Closed

Conversation

MrJamesEllis
Copy link

@MrJamesEllis MrJamesEllis commented Apr 11, 2024

Description

This PR is for some visual enhancements and modernisation of the CSS for display the various multivalue fields, and aligns the fields with other fields in the administration area (e.g. taking up full width of container)

The update:

  • Uses flexbox
  • Field aren't as narrow
  • Added grab icon and updated cursor for reordering
  • General CSS cleanup for readability

image

Manual testing steps

Apply the CSS, check for regressions. I haven't found any.

There may be some regressions where the field is used on the frontend, but developers should really be blocking/adding their own CSS there.

Issues

#105

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • [~] This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

@MrJamesEllis MrJamesEllis changed the title Feat field css Update field CSS Apr 11, 2024
@MrJamesEllis MrJamesEllis mentioned this pull request Apr 11, 2024
4 tasks
@GuySartorelli
Copy link
Collaborator

I notice you haven't checked some of the boxes in the checklist. Can you please update your PR so that it complies with all of the guidelines? If you are unsure about any of the points I'm happy to help.

@GuySartorelli
Copy link
Collaborator

@MrJamesEllis It's been a while since my last comment, are you still interested in getting this merged?

@GuySartorelli
Copy link
Collaborator

This hasn't had any action since my last comment, so I'm going to close it. If you want to work on it going forward feel free to @ me and I can reopen it.

@MrJamesEllis
Copy link
Author

HI @GuySartorelli - for whatever reason Github hasn't notified me of updates here, or it did and they never made it to me.
If you could reopen, that'd be great.

Regarding the unchecked task items, happy for you to provide some guidance on that as I don't think they apply in this case as it's just an enhancement to the visual layout of the field in the CMS.

@GuySartorelli
Copy link
Collaborator

Hi,

I will reopen after the relevant changes have been made. Regarding the checkboxes, they are all relevant except possibly the documentation one. Here's the rationale:

Checkbox reason
The commit messages follow our commit message guidelines Commit messages are used to generate the changelog, and different prefixes populate different sections of the changelog. The message itself is also important as a way to see broadly what a change was without looking at the code changes for every single commit.
The PR follows our contribution guidelines This one should be self-explanatory - you're making a contribution to an established open source project, so you should follow the established processes as much as possible. The CMS Squad is only 2 people right now, so we don't have a lot of time to look at community pull requests. If PRs already follow the process, it's a lot easier to get things merged.
Code changes follow our coding conventions Exactly the same rationale as with the contribution guidelines.
This change is covered with tests (or tests aren't necessary for this change) Most changes require tests. Some don't. If the change doesn't need tests, check the box. If it needs tests, add tests and then check the box.
Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release May not be relevant for this change - but many changes require changes to documentation
CI is green CI must be green for the PR to be merged - or at the very least any failures must be unrelated to the changes made in the PR.

@GuySartorelli
Copy link
Collaborator

Can you also please provide a clear "before" and "after" screenshot for this change, since it's only a visual change?

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.

2 participants