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

Primerize all backend flashes #16789

Merged
merged 14 commits into from
Sep 30, 2024
Merged

Primerize all backend flashes #16789

merged 14 commits into from
Sep 30, 2024

Conversation

oliverguenther
Copy link
Member

Ticket

https://community.openproject.org/work_packages/58003

What are you trying to accomplish?

Render all Rails flashes using the OpPrimer::FlashComponent now that it's possible

@oliverguenther oliverguenther force-pushed the chore/primerized-flashes branch 8 times, most recently from b752561 to 1d073a7 Compare September 26, 2024 09:13
@oliverguenther oliverguenther marked this pull request as ready for review September 26, 2024 09:13
Copy link
Member

@cbliard cbliard left a comment

Choose a reason for hiding this comment

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

Not sure where it comes from, but I found this issue when a flash is displayed: the title text of the error flash is joined.

With changes:

image

on dev:
image

Copy link
Member

@cbliard cbliard left a comment

Choose a reason for hiding this comment

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

Congrats 👏 It really cleans things up!

I have some minor remarks, and also a bit of confusion. For one, I'm not sure to understand the difference between flash and toast now. Is toast for angular only now?

spec/support/primerized_flash/expectations.rb Outdated Show resolved Hide resolved
spec/support/primerized_flash/expectations.rb Outdated Show resolved Hide resolved
Comment on lines 7 to 10
def expect_primerized_flash(message:, type: :success, wait: 20)
expected_css = expected_flash_css(type)
expect(page).to have_css(expected_css, text: message, wait:)
end
Copy link
Member

Choose a reason for hiding this comment

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

Another comment about #expect_primerized_flash. That's something that already bugged me with #expect_toast: I don't like that type: :success is assumed when using this helper. I think that if the type is not specified, then it should not be checked. I see that :default value will not make any assumption about the type. I think this should be the default value.

If a shorter version of #expect_primerized_flash(message:, type: :success) is needed, then we can introduce a #expect_primerized_success_flash(message:) like we already do for errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, but this should be a separate PR, it will break a lot of tests and will be easier to fix that way

Comment on lines -96 to +97
expect(page).to have_css(".op-toast.-info", text: I18n.t("js.ifc_models.empty_warning"))
show_default_page.expect_toast(type: :info, message: I18n.t("js.ifc_models.empty_warning"))
Copy link
Member

Choose a reason for hiding this comment

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

So toasts are still a thing? I'm not sure I get the difference between a flash and a toast. I thought everything was a flash now (rendered as a banner).

Copy link
Member Author

Choose a reason for hiding this comment

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

The op-toast classes are still used by angular

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if we want to clarify this in the examples? I just removed the primerized implementation detail, so not sure what to do with the angular part. As we will likely only remove toast expects, not add to them - maybe do nothing about it for now?

app/helpers/errors_helper.rb Show resolved Hide resolved
lookbook/docs/patterns/08-flash-banner.md.erb Outdated Show resolved Hide resolved
@oliverguenther oliverguenther force-pushed the chore/primerized-flashes branch 2 times, most recently from c72c542 to f480a56 Compare September 30, 2024 19:06
@oliverguenther oliverguenther merged commit 87ffaff into dev Sep 30, 2024
13 checks passed
@oliverguenther oliverguenther deleted the chore/primerized-flashes branch September 30, 2024 20:12
oliverguenther added a commit that referenced this pull request Sep 30, 2024
oliverguenther added a commit that referenced this pull request Sep 30, 2024
oliverguenther added a commit that referenced this pull request Oct 1, 2024
oliverguenther added a commit that referenced this pull request Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants