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

Move Login component to the Document namespace #2363

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Move Login component to the Document namespace #2363

wants to merge 1 commit into from

Conversation

jcoyne
Copy link
Contributor

@jcoyne jcoyne commented Feb 5, 2025

This makes it clear it's not a shared component

@dnoneill
Copy link
Contributor

dnoneill commented Feb 7, 2025

Why Document and not PDF or something along those lines? I know Document is the type in Purl but we use PDF more in the code

@jcoyne
Copy link
Contributor Author

jcoyne commented Feb 7, 2025

I think we should move toward the object type names to be consistent. We don't currently have a "Mp4" viewer, it's a "media viewer". We don't have a Jp2 viewer, we have a "image viewer".

This makes it clear it's not a shared component
@dnoneill
Copy link
Contributor

dnoneill commented Feb 7, 2025

Yeah but we don't have a document_viewer, we have a pdf_viewer. Maybe this should be a move to everything to document viewer. I guess my worry is it isn't consistent if there is only one thing that is named document.

@jcoyne
Copy link
Contributor Author

jcoyne commented Feb 7, 2025

@dnoneill I think that's a great idea as a follow up. But I don't want this PR to be large. I just want to show that this piece is not a shared component.

@dnoneill
Copy link
Contributor

dnoneill commented Feb 7, 2025

@jcoyne Okay but shouldn't it be consistent with what we currently have and not introduce an entirely new namespace we don't use anywhere else? I don't have a problem with this being small, if it is I think it should be PDF and then we can migrate to document.

@jcoyne
Copy link
Contributor Author

jcoyne commented Feb 7, 2025

@dnoneill we do have https://github.com/sul-dlss/sul-embed/blob/main/app/javascript/document.js already. We don't use the Pdf namespace anywhere.

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