Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Bug fixes and add warning to metadata standardizer for projects with 9+ columns #395

Merged
merged 8 commits into from
Sep 25, 2024

Conversation

sanghoonio
Copy link
Member

Is this too big to be a hotfix patch?

Things addressed (all small frontend things):

  • disable metadata standardizer for projects with 9+ cols
  • hide ph_id column in browse page
  • adjust how ph_id column is hidden in metadata standardizer modal
  • update POP interface styling (regardless, adding or removing PEPs from POPs seems broken in backend, not sure why)
  • hotfix views not working (fixed typo in types.ts)
  • fixed star count messing up when adding/removing PEPs from namespace page or updating a project in project page (couldn't figure out how to do it without useEffect)

@khoroshevskyi
Copy link
Member

update POP interface styling (regardless, adding or removing PEPs from POPs seems broken in backend, not sure why)

I think ui is not sending config file to the server:
Request URL: http://localhost:8000/api/v1/projects/khoroshevskyi/new-pop2341?tag=default
image

So I think It's trying to check correctness of config, but config doesn't exist. But I am not fully sure if I am right

@khoroshevskyi
Copy link
Member

disable metadata standardizer for projects with 9+ cols

I think it's not exactly correct solution, because most of geo/bedbase tables have more then 10 columns and we have the most interest in that tables.
@nsheff , what do you think about this solution?

Copy link

cloudflare-workers-and-pages bot commented Sep 23, 2024

Deploying pephub-ui with  Cloudflare Pages  Cloudflare Pages

Latest commit: c1e98ab
Status: ✅  Deploy successful!
Preview URL: https://20fcccaa.pephub.pages.dev
Branch Preview URL: https://dev.pephub.pages.dev

View logs

@sanghoonio sanghoonio changed the title Bug fixes and disable metadata standardizer for projects with 9+ columns Bug fixes and add warning to metadata standardizer for projects with 9+ columns Sep 25, 2024
Copy link
Member

@nleroy917 nleroy917 left a comment

Choose a reason for hiding this comment

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

Ok all looks good -- I think you can get rid of the useEffects quite easily

@@ -65,6 +65,13 @@ export const PepSearchDropdown = (props: Props) => {
value: `${n.namespace}/${n.name}:${n.tag}`,
})) || []
}
styles={{
Copy link
Member

Choose a reason for hiding this comment

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

Just curious if this is a common thing we add to dropdowns. If so, could add to a consts file and then import everywhere we use it

<p className='text-sm mt-3 mb-1'>
<strong>Note: </strong>
Your project has nine or more columns.
Due to server constraints, standardization of nine or more columns on PEPhub may take some time. Please be patient.
Copy link
Member

Choose a reason for hiding this comment

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

👍🏼👍🏼👍🏼

Comment on lines +26 to +29
useEffect(() => {
setLocalStarred(isStarred);
}, [project]);

Copy link
Member

Choose a reason for hiding this comment

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

And again here...

Copy link
Member Author

Choose a reason for hiding this comment

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

For some reason, the key={projectInfo.digest} didn't work here, so I'm gonna keep the useEffect for this one. I think it's because the project page refreshes differently than namespace when updating a project sample table.

@sanghoonio sanghoonio merged commit d59bd90 into master Sep 25, 2024
2 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants