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

Make sticky button not limited to Pages section #1294

Merged
merged 4 commits into from
May 30, 2024

Conversation

satrun77
Copy link
Contributor

@satrun77 satrun77 commented May 24, 2024

Description

The issue is within the elemental block. The CSS and JS for the sticky button expect the form fields to be displayed within the page's site tree view only. The top part of the fields is overlayed with the buttons bar and in mobile the buttons are visible and if you add many fields there is a big gap at the bottom of the page.

Screenshot 2024-05-24 at 11 05 46 PM

Manual testing steps

  • Install fresh CMS
  • Add user forms module.
  • Add elemental module https://github.com/dnadesign/silverstripe-elemental-userforms
  • Create a user form page and add a large number of fields (any type). The form works fine.
  • Create a page with an elemental block, add a form block with a large number of fields
  • The top part of the fields list is covered
  • scroll to the bottom of the page, and you will see a large empty space
  • resize the window to mobile, the buttons are not visible.

Issues

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

@satrun77 satrun77 marked this pull request as ready for review May 24, 2024 11:17
@satrun77
Copy link
Contributor Author

FYI. Assets build failing with this error. Looking into fixing it, any tip would be appreciated :)

Screenshot 2024-05-27 at 9 35 47 AM

@michalkleiner
Copy link
Contributor

I think that you basically need to have a kitchen sink setup with the admin module that provides these dependencies. It's always a black magic to me how to get the front-end assets compiled. @GuySartorelli will surely know better :)

@GuySartorelli
Copy link
Member

There is documentation about that - in particular the yellow callout block on that page should help.

@satrun77
Copy link
Contributor Author

I have sorted out the assets build, I removed the yarn lint from the the build command

@satrun77
Copy link
Contributor Author

Thanks @GuySartorelli That yellow box is what I needed to make it work without removing yarn lint

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Looks generally okay, except there's a weird 'hiccup' whenever I add a new field:
Screencast from 28-05-24 12:12:43.webm
Note how the fields start in the overlapped style, but then push down to where they should be.

Weirdly enough, this (or something like it) used to happen with userforms that aren't in an elemental form, but this PR fixes that.

If you can find a way to resolve that for both scenarios that would be awesome - if not just say so and I'll let it go as is, since this is overall an improvement.
Looks like this only happens in firefox, and it's only a very temporary blip, so if you can't figure out a clean solution for it quickly I'd say it's probably okay to leave in.

@satrun77
Copy link
Contributor Author

Looks generally okay, except there's a weird 'hiccup' whenever I add a new field: Screencast from 28-05-24 12:12:43.webm Note how the fields start in the overlapped style, but then push down to where they should be.

Weirdly enough, this (or something like it) used to happen with userforms that aren't in an elemental form, but this PR fixes that.

If you can find a way to resolve that for both scenarios that would be awesome - if not just say so and I'll let it go as is, since this is overall an improvement. Looks like this only happens in firefox, and it's only a very temporary blip, so if you can't figure out a clean solution for it quickly I'd say it's probably okay to leave in.

I will have a look at it and let you know tomorrow. 👌

@satrun77
Copy link
Contributor Author

@GuySartorelli Replaced the stick button with CSS implementation for better performance. I have tested that briefly, and it looks good.

@GuySartorelli
Copy link
Member

Looks like CI is getting a different css file output than what you've committed - can you please try running yarn build again?

@satrun77
Copy link
Contributor Author

yarn build

Fixed that

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

That's leagues better than it used to be, thanks for putting in the effort to improve that.

Works well locally, tested in both firefox and chromium - both with a userform page and elemental userform block.

@GuySartorelli GuySartorelli merged commit b429efb into silverstripe:6.2 May 30, 2024
13 checks passed
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.

3 participants