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

Reading room date picker #608

Merged
merged 12 commits into from
Feb 27, 2024

Conversation

chryslovelace
Copy link
Contributor

@chryslovelace chryslovelace commented Jun 7, 2023

This uses the endpoint added to request-broker in RockefellerArchiveCenter/request_broker#299 to get name and time information about available reading rooms from Aeon in order to ensure the user selects a valid time.

@chryslovelace
Copy link
Contributor Author

As it stands, the code here doesn't allow for a way to bypass the Aeon API integration that pulls in the reading room data. Is that something you would want, and if so, how would you want that to be configured?

@helrond
Copy link
Member

helrond commented Aug 17, 2023

@chryslovelace Right now we don't want users to have to pick a Reading Room or a time in our implementation of DIMES, so yes, it's something I'd like to be able to turn off. So far we've put all configuration stuff in .env but I'm wondering if there are better solutions for this that you'd suggest?

@chryslovelace chryslovelace force-pushed the aeon-dev branch 2 times, most recently from 764446c to 83b0689 Compare October 30, 2023 18:13
@chryslovelace chryslovelace requested a review from helrond November 3, 2023 18:02
@helrond
Copy link
Member

helrond commented Nov 10, 2023

Hi @chryslovelace - I'm gonna trust that the implementation for when you have reading rooms set up in Aeon works because we don't have that set up right now. HOWEVER I am seeing that the REACT_APP_ENABLE_READING_ROOM_SELECT is not working as expected, which I think is because that variable is being parsed as a string, not a boolean, so as long as it's present it evaluates to true. There are probably a number of ways to address this (only add the environment variable if you want to enable the reading room select, convert the string to a boolean, etc) but regardless of which you go with we should probably document this feature in the README.

@chryslovelace
Copy link
Contributor Author

As suggested, I converted that env var to boolean where its used, so it'll be activated as long as the env var is present and is anything but the empty string. The build process seems to be getting stuck in some sort of loop.

Copy link
Member

@helrond helrond left a comment

Choose a reason for hiding this comment

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

HI @chryslovelace I am baffled by the behavior in CI. My only thought is that something about the fact that this PR is coming from a different GH organization is causing the Docker build to blow up. 🤷

I haven't fully tested the reading room functionality because we're not going to be using it in the immediate future so I'll just trust that it works for you. I've added a couple of really minor things here. If you don't mind taking care of those I'll try cloning the development branch in this repository to another branch, repoint this PR at that branch and merge. That should allow me to point at development and have CI pass. Sound good?

.env Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
.env.deploy Outdated Show resolved Hide resolved
chryslovelace and others added 4 commits February 20, 2024 15:23
Co-authored-by: Hillel Arnold <helrond@hotmail.com>
Co-authored-by: Hillel Arnold <helrond@hotmail.com>
Co-authored-by: Hillel Arnold <helrond@hotmail.com>
@helrond helrond marked this pull request as ready for review February 27, 2024 14:45
@helrond helrond merged commit 781adf0 into RockefellerArchiveCenter:development Feb 27, 2024
1 check 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.

2 participants