Closed pjsier closed 7 years ago
Hmm, @pjsier, I don't see how to make the modal appear -- when I tested this locally the i-in-a-circle logo didn't appear next to the participant name, and when I clicked on the name it just opened an input field (as I should have expected). What should I expect to click on to see the popup?
Okay, I was able to see this (not sure what was going on originally, probably my fault!). The institution shows up as [object Object]
instead of as its name, but this looks great! Thanks, Pat. 👍
@cecilia-donnelly glad it's working for the most part! Just updated it to specify the name field on institution, so that should cover it
@pjsier Okay, now that I've tested a bit more I've found some strange behavior -- sorry for the slow review on my side. :/
The pop-up only appears (for me, in Firefox 52.3 and Chromium 60.0, on Debian 'buster') for attendees that have an associated institution. When I click into an action I see the i-in-a-circle logo next to the name of each participant, but when I click on it nothing happens except for those that have an institution.
When I click a participant without an institution (with no result) and then a participant with an institution I see a compound popup: a few fields from the no-institution participant and then the full set for the participant with an institution, as in this screenshot:
The compounding continues for as many institution-lacking participants I click on.
On a smaller note, do you think we should say something about editing in the "See more details" link? Maybe not, since they can already edit the primary fields directly in the sign-in sheet.
That should fix it! Ran into some weirdness with the merge conflict, but the institution display was just missing a null check so it works in that use case now
No need to stress about it, but git rebase
and then git push --force
can be your friend for those conflicts. While it's a really bad idea to rewrite history on shared branches, for one like this that you're working on alone those commands help clean up the history so that commands like git blame
and git show
are more useful later.
Is this ready for me to re-test or is there more coming?
If you're good with the "See more details" as is then it's ready to re-test (didn't respond to that, but I couldn't think of a good way of phrasing it).
And I can rebase it if that's preferred, but I wasn't sure because of the shared history. Do you want me to rebase?
Fair! Would you rebase it please? We've been trying out a system here in which branches that people are working on alone are okay for force-pushing, though shared branches aren't and master never is. (I'm sorry for assuming you hadn't thought about it, and thank you very much for defaulting to not-forcing!)
Makes sense, and sure! Should I rebase just that last fix or the whole PR to get it in one commit?
I like to keep the logical changes in a PR in separate commits and just rebase to avoid merge commits.
Sorry about the messiness, but I got the other two commits rebased only to run into some trouble on merging in the updates. Let me know if this works!
This looks great! Thanks, @pjsier. Merging now.
Should fix #308. I tried to stick with the conventions of how the code is currently set up as much as possible (using
inline_form_config
for the form display, creating a new partial template and including that in the form withblock.super
).Institutions should still be a link to the new tab rather than a pop-up. Also wanted to flag that I made an update to the information icon so that it uses
display: none
instead ofvisibility: hidden
, because usingvisibility
left extra spacing in cells without the icon.Edit: Added screenshot below of the pop-up