-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: slim application state UI to non-handler users #2951
Conversation
(when callback | ||
(callback))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this allows bootstrap element to communicate state back to reagent component so that UI can render "hidden/show" elements in correct order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may also want to consider a new implementation of the collapsible with Bootstrap 5 or so. Maybe CSS animations + React implementation is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Let's make sure we have issue for the eventual refactoring.
src/cljs/rems/application.cljs
Outdated
(defn- application-state [application config highlight-request-id] | ||
(defn- render-state-fields [application config events] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of preferred the older naming convention of application-state
component instead of a "render function". Would application-state-details
communicate that this is the details that we have initially hidden? Same logic for the others.
(when-not (false? bottom-less-button?) show-less)]) | ||
content-footer])) | ||
(defn- block [{:keys [open?]}] | ||
(let [show? (r/atom open?)] ; track internal open/closed status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of duplicates what was previously handled by the classes/ids. Would be good to refactor this into the atom completely (with CSS animation for opening/closing). Doesn't have to happen now, but we can make sure we have an issue for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, currently bootstrap is handling the animation. i'll create a ticket for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
relates #2871
Checklist for author
Remove items that aren't applicable, check items that are done.
Reviewability
Documentation
Testing
Follow-up
Screenshots
as applicant, collapsed state and member blocks
![Screenshot 2022-06-07 at 12 02 02](https://user-images.githubusercontent.com/794087/172341064-f3e9553d-5e6b-43eb-9039-d95a5e73e665.png)
expanded
![Screenshot 2022-06-07 at 11 16 26](https://user-images.githubusercontent.com/794087/172331270-0e9ebfa5-4254-4229-a254-bcb5b574c640.png)