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

Added styled component to AddToCollectionList component #2368

Conversation

adityagarg06
Copy link
Contributor

Changes:
Added styled component to AddToCollectionList component.

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

@adityagarg06
Copy link
Contributor Author

adityagarg06 commented Aug 8, 2023

Hi @lindapaiste, I didn't see your PR #2353 before making this PR. I will make the changes later when you will merge the new code of AddCollectionList and AddCollectionSketchList components. We can put this as a potential fix as these styled components can be used in the new code.

@lindapaiste
Copy link
Collaborator

@adityagarg06 Sure thing. We can merge either my PR or yours first and then fix the conflicts that show up. Realistically your PR gets merged first.

We have two components which are really similar (and it's confusing which is which!) but they both use these same two CSS classes which you converted. Maybe we do some sort of shared wrapper at some point, but for now I think you should export your two styled components and also use them in the AddToCollectionSketchList.jsx. Then you can delete those classes from the CSS.

If we want to make an improvement to the CSS while we're in here, how about we change it from:

min-width: ${remSize(600)};

to:

width: ${remSize(600)};
max-width: 100%;

That will help a lot with responsiveness.

Copy link
Collaborator

@lindapaiste lindapaiste left a comment

Choose a reason for hiding this comment

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

I'm playing around with the github suggestions tool!


const projectInCollection = (project, collection) =>
collection.items.find((item) => item.projectId === project.id) != null;

const CollectionAddSketchWrapper = styled.div`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const CollectionAddSketchWrapper = styled.div`
export const CollectionAddSketchWrapper = styled.div`


const projectInCollection = (project, collection) =>
collection.items.find((item) => item.projectId === project.id) != null;

const CollectionAddSketchWrapper = styled.div`
&&& {
min-width: ${remSize(600)};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
min-width: ${remSize(600)};
width: ${remSize(600)};
max-width: 100%;


const projectInCollection = (project, collection) =>
collection.items.find((item) => item.projectId === project.id) != null;

const CollectionAddSketchWrapper = styled.div`
&&& {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't really matter but I suspect that you'd be fine without the &&&?


const QuickAddWrapper = styled.div`
&&& {
min-width: ${remSize(600)};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
min-width: ${remSize(600)};
width: ${remSize(600)};
max-width: 100%;

Copy link
Collaborator

@lindapaiste lindapaiste left a comment

Choose a reason for hiding this comment

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

Perfect! Thank you.

@lindapaiste lindapaiste merged commit 0bd139b into processing:develop Aug 13, 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.

2 participants