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

Fix Contact Type Select Styles Issue #5759 #5830

Merged
merged 7 commits into from
Jun 23, 2024

Conversation

logtay
Copy link
Contributor

@logtay logtay commented Jun 11, 2024

What github issue is this PR for, if any?

Resolves #5759

What changed, and why?

The first letter in the dropdown is now in line with the rest of the word so that options are properly selected. Only the first letter of the searched contact type was getting selected, which would make it difficult for users to read. These changes affect the casa_cases and case_contacts pages

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

Changes were only made to CSS, so I did not create any new tests.

Screenshots please :)

Run your local server and take a screenshot of your work! Try to include the URL of the page as well as the contents of the page.
Screen Shot 2024-06-10 at 19 28 03
Screen Shot 2024-06-10 at 19 28 47

Feelings gif (optional)

What gif best describes your feeling working on this issue? https://giphy.com/
How to embed:

alt text

Copy link
Collaborator

@elasticspoon elasticspoon 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 fix! The styles work great! 🙂

The only change I would like to see is to place this behavior in a slightly different place.

Currently we are using tomselect as a component so ideally we want to be able to reuse it on other pages so we should but the styles somewhere in /app/assets/stylesheets/shared to reflect that the styles are not page specific. Maybe similar to how the select2 styles live there.

Also in that situation we should also use a selector more tailored to the tom-select drop down maybe .ts-dropdown [data-selectable] .highlight which is what tomselect already uses.

@logtay
Copy link
Contributor Author

logtay commented Jun 17, 2024

Thank you for the feedback! I will make those changes

Copy link
Collaborator

@elasticspoon elasticspoon left a comment

Choose a reason for hiding this comment

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

Since you added app/assets/stylesheets/shared/tomselect_additional_styles.scss. We no longer need app/assets/stylesheets/pages/casa_cases.scss or app/assets/stylesheets/pages/case_contacts_form.scss.

Although those files sound like they might only be loaded on those pages, they actually get loaded on all pages so the styling from app/assets/stylesheets/shared/tomselect_additional_styles.scss is enough.

@elasticspoon
Copy link
Collaborator

@logtay hey, sorry I explained really poorly. I'm not tryna jerk you around or anything. 🙁

when i said

Since you added app/assets/stylesheets/shared/tomselect_additional_styles.scss. We no longer need app/assets/stylesheets/pages/casa_cases.scss or app/assets/stylesheets/pages/case_contacts_form.scss.

I didn't mean for you to delete the entire files, I meant just the changes you made to them. So basically all this PR needs to contain is the line that imports the new stylesheet + the new stylesheet.

@logtay
Copy link
Contributor Author

logtay commented Jun 22, 2024

@elasticspoon It's okay, I think I misread. I thought I got rid of those first changes, but had some issues on my end, so probably overlooked it. Let me go ahead and do that. Thank you for your patience, I appreciate it.

Copy link
Collaborator

@elasticspoon elasticspoon left a comment

Choose a reason for hiding this comment

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

Nice job! It works.

@elasticspoon elasticspoon merged commit c94f90c into rubyforgood:main Jun 23, 2024
18 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.

Fix Contact Type Select Styles
2 participants