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

Update bootstrap researchplans #2052

Merged

Conversation

ashwiniHerle
Copy link
Contributor

@ashwiniHerle ashwiniHerle commented Aug 2, 2024

This PR covers react-bootstrap upgrade for the components under:

  • apps/mydb/elements/details/researchPlans/*.js:

    researchPlanTab/*
    analysesTab/*
    attachmentsTab/*

  • components/structureEditor/StructureEditorModal.js

Components under apps/mydb/elements/details/researchPlans/wellPlatesTab are not covered in this PR

@ashwiniHerle ashwiniHerle self-assigned this Aug 2, 2024
Copy link
Collaborator

@beque beque left a comment

Choose a reason for hiding this comment

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

Just some minor fixes ;-)

@@ -106,5 +106,6 @@ $theme-colors: map-merge($theme-colors, $gray-tones);
}

.border-dashed {
border-style: dashed;
border: dashed;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Border is a shorthand for the properties border-width, border-style and border-color.
This class should be like the bootstrap classes for border-width (border-2)
I do not think it is a good idea to change this class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason, it did not work with just border-dashed class. I will look into it again

);
}

const text = (data || {}).text || name;

return (
<span className='fs-3'>{text.toUpperCase()}</span>
<span className='fs-4'>{text.toUpperCase()}</span>
);
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have extended this component in my screen branch too.
Unfortunately this changes of font size will take effect in the icons in the quill editor.

That's what I have changed:
39b3856
If you want you can adopt this in this branch
Or I will do it when I rebase my branch

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'll adopt your method and check all the occurrences with it

</Col>
<Col smOffset={0} sm={2}>
<Form.Label>Action</Form.Label><br />
<Button variant="danger" className="pull-right" size="sm" onClick={() => this.removeResearchPlanMetadataArrayItem('description', index)}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

As I remember it correct.
className="pull-right" should be removed
There are several places in this file
Just test it if it still works

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, it still works. I found few places where normal justify-content and similar classes didn't work and pull-right has worked.

))
}
</Dropdown.Menu>
</Dropdown>
</ButtonGroup>
</OverlayTrigger>
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

here are some pull-right classes in this file

</div>
);
} else {
content = <p>Drop Files, or Click to Select.</p>;
}
return (
<div>
<FormGroup style={{ width: '30%' }}>
<Form.Group style={{ width: '30%' }}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you can try the col classes for the width
https://getbootstrap.com/docs/5.3/layout/grid/

@@ -154,7 +154,7 @@ class ResearchPlanDetailsFieldTableMeasurementExportModal extends Component {
</Modal.Title>
</Modal.Header>
<Modal.Body className='measurementExportModal__body'>
<table className="table">
<Table className="table">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this class "table" necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not. Will make necessary changes

<ButtonToolbar>
</Table>
</Modal.Body>
<Modal.Footer className="modal-footer border-0">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this class "modal-footer" necessary?

@@ -18,21 +17,12 @@ export default class ResearchPlanDetailsName extends Component {
>
<Button
id="copyMetadataButton"
title=""
className="fa fa-laptop pull-right"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another pull-right class

@@ -197,4 +197,3 @@
display: none
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

You have removed many classes in the components.
Are there any classes left from this file?
If not, you could delete this file

Copy link
Collaborator

@maiwald maiwald left a comment

Choose a reason for hiding this comment

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

Some minor notes 😊

@@ -161,7 +154,7 @@ export default class ResearchPlanDetailsFieldTable extends Component {
onChange(field.value, field.id);
}

handleSchemaModalShow() {
handleSchemaModalShow = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the difference between this and the previous method declaration? Just trying to understand the reasoning. 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use .bind in many places. Got rid of the .bind after using arrow function

<Dropzone
accept="image/*"
multiple={false}
onDrop={(files) => this.handleDrop(files)}
className="border-dashed border-gray-300 text-center p-2 mb-3"
>
{isAnnotationUpdated && <SaveEditedImageWarning visible />}
Copy link
Collaborator

Choose a reason for hiding this comment

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

If <SaveEditedImageWarning> is not used anywhere else, we might as well get rid of the visible prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used in other components as of now

@@ -146,7 +146,7 @@ export const annotateButton = (attachment, parent) => (
<ImageAnnotationEditButton
parent={parent}
attachment={attachment}
className={`attachment-button-size ${!isImageFile(attachment.filename) ? 'attachment-gray-button' : ''}`}
className={`attachment-button-size ${!isImageFile(attachment.filename) && 'attachment-gray-button'}`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will add a class named false if !isImageFile(attachment.filename) evaluates to false.

@@ -181,7 +181,7 @@ export default class ResearchPlanDetailsContainers extends Component {
const contentOneLine = {
ops: content.ops.map((x) => {
const c = Object.assign({}, x);
if (c.insert) c.insert = truncateText(c.insert.replace(/\n/g, ' '), 300);
if (c.insert) c.insert = truncateText(c.insert.replace(/\n/g, ' '), 100);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can extract truncateText into a separate file, as there is now one instance in this file and in ReactionDetailsContainers.js? Or would truncating in CSS be an option? Not sure whether that is better here, though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Moved to textHelper and reused it.

<Card.Body>
<p>This sample has parents or descendants, and they will not be changed.</p>
<p>Are you sure?</p>
<br />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we get rid of this <br />?

@ashwiniHerle ashwiniHerle merged commit 68627aa into update-bootstrap-ui-library Aug 5, 2024
2 of 3 checks passed
ashwiniHerle added a commit that referenced this pull request Aug 13, 2024
update bootstrap: researchplans and structure editor modal
@ashwiniHerle ashwiniHerle deleted the update-bootstrap-researchplans branch August 15, 2024 09:11
maiwald pushed a commit that referenced this pull request Aug 27, 2024
update bootstrap: researchplans and structure editor modal
ashwiniHerle added a commit that referenced this pull request Aug 28, 2024
update bootstrap: researchplans and structure editor modal
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