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

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

web/src/components/modals/remove-pep-from-pop.tsx Outdated Show resolved Hide resolved
<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.

👍🏼👍🏼👍🏼

web/src/components/pop/pop-card-dropdown.tsx Outdated Show resolved Hide resolved
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
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.

3 participants