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

Release 97.0 #526

Merged
merged 42 commits into from
Nov 8, 2023
Merged

Release 97.0 #526

merged 42 commits into from
Nov 8, 2023

Conversation

matthiasblum
Copy link
Contributor

@matthiasblum matthiasblum commented Oct 31, 2023

Includes the following changes:

matthiasblum and others added 30 commits September 13, 2023 10:11
# Conflicts:
#	src/components/Entry/Summary/index.tsx
#	src/custom.d.ts
# Conflicts:
#	src/components/Entry/Summary/SidePanel/index.tsx
@coveralls
Copy link

coveralls commented Oct 31, 2023

Coverage Status

coverage: 23.472% (-0.04%) from 23.512%
when pulling 08014f2 on dev
into dcc6399 on master.

Use pre-selected representative domains
@matthiasblum matthiasblum marked this pull request as ready for review November 6, 2023 10:09
Copy link
Member

@gustavo-salazar gustavo-salazar left a comment

Choose a reason for hiding this comment

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

I left some minor comments/questions, but nothing blocking this to get merge!

text: string;
};

class DescriptionLLM extends PureComponent<Props> {
Copy link
Member

Choose a reason for hiding this comment

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

Nothing wrong with it, but wondering why do you prefer PureComponent here instead of a functional component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe I based the component on the Description one which extends PureComponent.

Copy link
Member

Choose a reason for hiding this comment

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

Right back at me!! 🤦🏽‍♂️


return (
<>
<div className={css('row')}>
Copy link
Member

Choose a reason for hiding this comment

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

OK for now, but my idea of the migration towards the visual framework and out of foundation, also includes not using the column/row strategy but rather flex, grid or other layouts available directly in CSS.

I'll put a note on this and will change the classes later when working on the TS migration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I went for the simplest solution, but indeed this needs to be changed for a better layout system.

/>
</>
);
} else if (metadata.integrated) {
Copy link
Member

Choose a reason for hiding this comment

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

So a description from the integrated signature is preferred over one AI generated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Right now, we don't have any integrated signature with AI-generated descriptions, but if we ever have these cases we should prioritise descriptions provided by member databases (unless we have evidence that AI-generated descriptions are better).

@@ -10,6 +10,14 @@
margin: 4px;
z-index: var(--z-index-tooltips);

&.fixed {
Copy link
Member

Choose a reason for hiding this comment

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

was this trying to fix the overgrown tooltip? is it still necessary after my fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Feel free to remove it now that your fix addresses the overgrown tooltip.

@@ -45,6 +48,14 @@ const Tooltip = ({
arrow({
element: arrowRef,
}),
size({
Copy link
Member

Choose a reason for hiding this comment

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

@matthiasblum this is how I'm trying to solve the issue about the overgrown tooltip. It seems to work well here and in other cases , but let me know if you notice the tooltip misbehaving somewhere else

@matthiasblum matthiasblum merged commit 3b05af0 into master Nov 8, 2023
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