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

Improve report generation modal #5931

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wanguiwaweru
Copy link

What github issue is this PR for, if any?

Resolves #5888

What changed, and why?

Currently, the dropdown input in the modal to generate court report documents is not easily visible and users don't get an alert if they try to generate a report without selecting a case.
This PR improves the modal by :

  • Adding a border to the dropdown input to make it more visible.
  • Used Javascript to make the select required.

How is this tested? (please write tests!) 💖💪

Note: if you see a flake in your test build in github actions, please post in slack #casa "Flaky test: " :) 💪
Note: We love capybara tests! If you are writing both haml/js and ruby, please try to test your work with tests at every level including system tests like https://github.com/rubyforgood/casa/tree/main/spec/system

Screenshots please :)

Add border

Comment on lines +1088 to +1093
.input-style-1 select{
width: 465px;
height: 40px;
border-radius: 5px;
border: 1px solid #4a6cf7;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This bit isn't actually showing up for me

image

I think it is because you are styling the select whereas you need to style the span that the select2 dropdown generates. Also it would be better if you could do the styles with bootstrap CSS classes. https://getbootstrap.com/docs/5.3/utilities/borders/#radius stuff line that.

Comment on lines +50 to +52
if(!$('#case-selection').val()){
alert('Select an active case and specify the date range.');
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

honestly i don't love how the default alert looks, I would like it to highlight the field that has the error.

I would be fine with this if I can be done as an HTML attribute otherwise lets just ditch the client side JS requirement.

Copy link

This PR has been open for a long time without any pushes or comments! What's up?

@wanguiwaweru
Copy link
Author

This PR has been open for a long time without any pushes or comments! What's up?
Please reassign the issue. I haven't been able to get to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

improve report generation modal from sidebar
2 participants