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

Print files owned by current user to console #347

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Conversation

KevinDCarlson
Copy link
Collaborator

@KevinDCarlson KevinDCarlson commented Jan 27, 2025

This PR adds a route /user/:username accessing a public profile page for user username which lists their documents. Currently it simply lists all owned models but the intended design is that it lists all public documents of any class. The display is currently just of hyperlinked titles; the intent is to include more metadata.

@KevinDCarlson KevinDCarlson marked this pull request as draft January 27, 2025 19:18
@KevinDCarlson KevinDCarlson marked this pull request as ready for review January 27, 2025 22:09
Copy link
Member

@epatters epatters left a comment

Choose a reason for hiding this comment

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

Thanks for starting this, Kevin. I left a few quick comments, but there are bigger designs issue that we need to address:

  1. We shouldn't list just any documents that the user's owns: what if they're supposed to be private? We should implement Published flag for documents #291 first and then only list documents that have been marked as published. Sub has emphasized this and n-dim had a similar approach, which I think we should follow.

  2. The user profile editable by a logged-in user should be a separate page from the public display of a profile visible to anyone. This will also avoid that weird stuff with refs.

<UserProfileForm />
<UserProfileForm onRefsLoaded={setMyRefs} />
<div style="margin-top: 1rem;">
<h3
Copy link
Member

Choose a reason for hiding this comment

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

No inline styles. Use the external stylesheet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should/can we lint directly for the keyword style, then, if we're not allowing even one-liners?

"0 2px 8px rgba(0,0,0,0.1)";
}}
onMouseOut={(e) => {
e.currentTarget.style.boxShadow = "none";
Copy link
Member

Choose a reason for hiding this comment

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

Ditto about inline styles, and also you shouldn't use mouse over events to change styles. Use the appropriate CSS selectors.

{items.map(([id, title]: [string, string]) => (
<Fragment key={id}>
<a
href={`${import.meta.env.VITE_SERVER_URL}/model/${id}`}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't need this env variable. Use Solid Router's A component.

/** Page to configure user profile. */
export default function UserProfilePage() {
const [myRefs, setMyRefs] = createSignal<[string, string][] | null>(null);
Copy link
Member

Choose a reason for hiding this comment

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

Having to pass these refs around is a code smell. Also, I don't understand the types. This isn't how you usually create a ref.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The refs are going to be a list of [string,string]s giving a document's ref and name. Is the problem that it's already been stringified rather than passing around actual document objects, or something like that?

@KevinDCarlson
Copy link
Collaborator Author

Thanks @epatters. I agree that there should be a public user page that shows only public docs. Maybe the page where you can edit your profile can be a "private" profile page where you can also see your private owned docs?

@epatters epatters marked this pull request as draft January 27, 2025 23:51
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think RpcResult<Array<[string, string]>>> is probably wrong here and reflects the weird front-end types you couldn't parse earlier.

const api = useApi();
const params = useParams();

const [userModels, setUserModels] = createSignal<[string, string][]>(); //QQQ: unclear what type the RPC call should return to; Documents?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See first comment for the Rust side of this question.

@KevinDCarlson
Copy link
Collaborator Author

KevinDCarlson commented Jan 28, 2025

OK, I think I was able to sensibilize the work so far pretty easily. But there are some todos:

  • Should the model list display more than just the title? Probably the theory, maybe the most recently edited time, and it should probably be sorted by the latter (later, options for how to sort, etc)
  • Need to let people choose which docs are public as in Published flag for documents #291
  • After deciding what the RPC function should really be returning rather than just the stringified UUID and name (the whole content JSON blob?), that needs to be reimplemented.
  • Partly because of the insufficient design above, this currently only lists models. The page should list the document type. Conceivably these points depend on Systematize types for documents and links between them #318 , to convert the JSON blobs into general Documents as they come into the front-end?
  • Probably future work but the document list should probably be a standalone component which can be configured with permissions settings and then display public documents on the public profile page and all documents on the private profile page.
  • There should be a link to your own public profile when you're logged in.
  • Separate issue, but you should be able to put a bio into your public profile page.

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