Skip to content
This repository was archived by the owner on Apr 24, 2024. It is now read-only.

1122 drawing layer fill patterns#1272

Open
danielsteinkogler wants to merge 37 commits into
masterfrom
1122-drawingLayer-fillPatterns
Open

1122 drawing layer fill patterns#1272
danielsteinkogler wants to merge 37 commits into
masterfrom
1122-drawingLayer-fillPatterns

Conversation

@danielsteinkogler

Copy link
Copy Markdown
Contributor

Basics

  • The PR is rebased with current master
  • I added a line to changelog.md
  • Details of what I changed are in the commit messages
  • References to issues, e.g. close #X, are in the commit messages and changelog
  • The buildserver is happy

Checklist

  • I fully described what my PR does in the documentation
  • I fixed all affected documentation
  • I fixed the introduction tour
  • I wrote migrations in a way that they are compatible with already present data
  • I fixed all affected decisions
  • I added automated tests or a manual test protocol
  • I added code comments, logging, and assertions as appropriate
  • I translated all strings visible to the user
  • I mentioned every code or binary not directly written or done by me in reuse syntax
  • I created left-over issues for things that are still to be done
  • Code is conforming to our Architecture
  • Code is conforming to our Guidelines
  • Code is consistent to our Design Decisions
  • Exceptions to any guidelines are documented

First Time Checklist

Review

  • I've tested the code
  • I've read through the whole code
  • I've read through the whole documentation
  • I've checked conformity to guidelines
  • I've checked conformity to requirements
  • I've checked that the requirements are tested

@danielsteinkogler danielsteinkogler linked an issue Apr 11, 2024 that may be closed by this pull request
2 tasks
@danielsteinkogler
danielsteinkogler force-pushed the 1122-drawingLayer-fillPatterns branch from 389ccba to ef9ee61 Compare April 11, 2024 20:10
@danielsteinkogler
danielsteinkogler marked this pull request as ready for review April 11, 2024 20:12
@horenso

horenso commented Apr 12, 2024

Copy link
Copy Markdown
Contributor

please add a summary :)

@horenso

horenso commented Apr 16, 2024

Copy link
Copy Markdown
Contributor

I fixed the migrations =)

@danielsteinkogler danielsteinkogler self-assigned this Apr 17, 2024
@danielsteinkogler danielsteinkogler added the please review Review by unspecified person requested label Apr 17, 2024
Comment thread doc/changelog.md Outdated
Comment thread doc/decisions/backend_orm_crate.md Outdated
Comment thread frontend/src/features/map_planning/store/UntrackedMapStore.ts Outdated

@hatchla hatchla left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Works really great for me, good work! See comments I made for some minor issues I found.

  • I've tested the code
  • I've read through the whole code
  • I've read through the whole documentation
  • I've checked conformity to guidelines
  • I've checked conformity to requirements
  • I've checked that the requirements are tested

@hatchla

hatchla commented Apr 18, 2024

Copy link
Copy Markdown
Contributor

When testing (using Firefox) I noticed a bar on the bottom of the buttons when selecting a type, which goes away after clicking somewhere else.
See the screenshot:
grafik

@badnames badnames left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I did not find any major issue but keep in mind I have not tested this PR locally yet.

Comment thread frontend/src/components/Form/DebouncedSimpleFormTextArea.tsx Outdated
Comment thread frontend/src/features/map_planning/layers/drawing/labels/EditableTextinput.tsx Outdated
}

export function generatePointsPatternAsImage(color: string) {
const svgString = `<svg width="100" height="100" xmlns="http://www.w3.org/2000/svg">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a specific reason why you hard coded this svg data?
I would prefer it if you saved it to its own file, importing it like we do with our icons.

This would make it easier to edit fill patterns in the future, especially for non-technical team members such as @yvonnemarkl.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

you are right, i also don't really like the current solution.
i had some struggles with importing the svg and then create a html image as konva is expecting.
However, I will take a further look into this.

@danielsteinkogler
danielsteinkogler force-pushed the 1122-drawingLayer-fillPatterns branch from db7bbf1 to 7df9a03 Compare April 23, 2024 12:40
@danielsteinkogler

Copy link
Copy Markdown
Contributor Author

jenkins build please

1 similar comment
@danielsteinkogler

Copy link
Copy Markdown
Contributor Author

jenkins build please

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

please review Review by unspecified person requested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Drawing Layer: Change fill_enabled to fill_pattern

5 participants