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

Expose flash keys ERROR and SUCCESS #468

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Expose flash keys ERROR and SUCCESS #468

wants to merge 2 commits into from

Conversation

cies
Copy link
Member

@cies cies commented Oct 11, 2024

We had them defined locally, which it weird: they should come from the source.

@cies cies self-assigned this Oct 11, 2024
@cies cies requested a review from asolntsev October 23, 2024 11:18
Copy link
Contributor

@asolntsev asolntsev left a comment

Choose a reason for hiding this comment

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

Probably we should expose not these constants, but METHODS forbreading/adding these flash values?

@cies
Copy link
Member Author

cies commented Nov 5, 2024

Probably we should expose not these constants, but METHODS forbreading/adding these flash values?

@asolntsev I disagree. there are standard methods for that, like .contains(...) and .get(...). Sure it would be nice to have, but since this is already how we've worked until now, i really believe simply exposing them is the right thing to do.

We currently have those same constants in our code, simply because they are not exposed and i dont like to use stings everywhere. :)

@cies cies added this to the 2.6.4 milestone Nov 5, 2024
@asolntsev
Copy link
Contributor

Probably we should expose not these constants, but METHODS forbreading/adding these flash values?

@asolntsev I disagree. there are standard methods for that, like .contains(...) and .get(...). Sure it would be nice to have, but since this is already how we've worked until now, i really believe simply exposing them is the right thing to do.

We currently have those same constants in our code, simply because they are not exposed and i dont like to use stings everywhere. :)

Can you share a code sample - how do you use those constants?

@cies
Copy link
Member Author

cies commented Nov 6, 2024

@asolntsev suuure:

            flash.get(ERROR)?.let {
              div("hint hint--error") { +it }
            }
            flash.get(SUCCESS)?.let {
              div("hint hint--optional") { +it }
            }
div(if (flash.contains(SUCCESS) || flash.contains(ERROR)) "low-top-margin" else "") {
  // ...
}

@asolntsev
Copy link
Contributor

Thank you. This is what I meant.
Instead of publishing constants ERROR and SUCCESS, it's better to publish methods like flash.getErrorMessage(), flash.getSuccessMessage(). Then it will be possible in the future to change the implementation details.

(though, it's not very important because internals of "flash" are already leaked anyway...)

@cies
Copy link
Member Author

cies commented Nov 22, 2024

@asolntsev Updated it, have a look

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

Successfully merging this pull request may close these issues.

2 participants