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

Bootstrap upgrade bugfixes 2 #2189

Open
wants to merge 18 commits into
base: update-bootstrap-ui-library
Choose a base branch
from

Conversation

ashwiniHerle
Copy link
Contributor

@ashwiniHerle ashwiniHerle commented Sep 30, 2024

Minor styling fixes related to bootstrap upgrade.
Made analyses look similar throughout samples, reactions and researchplans, wellplates and cellLines
Fix for issue where contextActions buttons vertically expanded in smaller screens
Show notification message numbers on NoticeButton

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 small fixes.

I have seen that the Analyses Header of CellLine and Wellplate still look different

<div className="tabs-container--with-borders">
<div
// className="tabs-container--with-borders"
>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this class be removed?

<ReactionDetailsContainers
reaction={reaction}
readOnly={!permitOn(reaction)}
handleSubmit={this.handleSubmit}
handleReactionChange={this.handleReactionChange}
/>
</ListGroupItem>
{/* </ListGroupItem> */}
</Tab>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can the ListGroupItem be removed?

@@ -352,56 +351,72 @@ export default class ReactionDetailsContainers extends Component {
));

if (analyses_container.length === 1 && analyses_container[0].children.length > 0) {


Copy link
Collaborator

Choose a reason for hiding this comment

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

Minimum one empty line too much

Copy link
Collaborator

Choose a reason for hiding this comment

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

Button in Analyses Header have no gap

{analysesContainer[0].children.map((container, key) => {
if (container.is_deleted) {
return (
<Accordion.Item
eventKey={key}
key={`research_plan_container_deleted_${container.id}`}
className="border rounded overflow-hidden"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The border is too big. Maybe border rounded is not necessary here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Buttons in Analyses Header have no gap

@ashwiniHerle
Copy link
Contributor Author

Have made necessary changes based on feedback and also fixed few undefined errors in cellLines.

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 2 analyses components with double border and one question.
When you fixed this you can merge this PR.

The navigation buttons looking good on laptop and big screen

/>
</Accordion.Body>
</Accordion.Item>
<Card eventKey={container.id}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a double border around the analyses header.
If you add border-0 at <Card> it should look good
Other analyses have the classes border-0 rounded-0

return (
<Accordion.Item
<Card
eventKey={key}
key={`research_plan_container_${container.id}`}
>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding class border-0 removes double border
Other analyses have the classes border-0 rounded-0

@@ -3,16 +3,16 @@ import { observer } from 'mobx-react';
import { Button, Badge } from 'react-bootstrap';
import { StoreContext } from 'src/stores/mobx/RootStore';

const SampleTaskNavigationElement = ({}) => {
function SampleTaskNavigationElement() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why have you changed this?
const SampleTaskNavigationElement = ({}) => is correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were linting errors. I'll undo it

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