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

Add usage of letters for rendering FP SVG. #79

Merged
merged 16 commits into from
Apr 11, 2024
Merged

Add usage of letters for rendering FP SVG. #79

merged 16 commits into from
Apr 11, 2024

Conversation

pszulczewski
Copy link
Contributor

This draft adds only SVG rendering options to use letters or numbers for the grid numbering axis labels.
Selecting letters for the X axis will make the FP look like a chessboard.
obraz

The downside is that for adding tiles it still uses numbers, because FloorPlanTile model x_origin and y_origin fields are now PositiveSmallIntegerField.

obraz

Potentially form could be changed to present numbers or letters for tile origins, and under clean() it could be covered back to numbers.

@joewesch
Copy link
Contributor

Potentially form could be changed to present numbers or letters for tile origins, and under clean() it could be covered back to numbers.

I like this idea. Let's do it!

nautobot_floor_plan/svg.py Outdated Show resolved Hide resolved
nautobot_floor_plan/svg.py Outdated Show resolved Hide resolved
nautobot_floor_plan/svg.py Outdated Show resolved Hide resolved
@itdependsnetworks
Copy link
Contributor

I had mentioned that we should consider having a generator for the increment #8 (comment)

@pszulczewski
Copy link
Contributor Author

The latest commit is to support letters in the tile form.

obraz

@itdependsnetworks I am fine with implementing generator if @joewesch allows more time.

@joewesch
Copy link
Contributor

I had mentioned that we should consider having a generator for the increment

I like the idea, but I think what we have so far will be a good start as I believe it covers at least 80% of use cases. Let's count this as incremental progress toward the eventual goal and not mark this as closing/resolving the issue.

@pszulczewski
Copy link
Contributor Author

I am still thinking if keeping x/y grid setting per object will give good user experience? Maybe it should be a global plugin setting, which is selected only once?

nautobot_floor_plan/forms.py Outdated Show resolved Hide resolved
nautobot_floor_plan/forms.py Outdated Show resolved Hide resolved
@joewesch
Copy link
Contributor

I am still thinking if keeping x/y grid setting per object will give good user experience? Maybe it should be a global plugin setting, which is selected only once?

Yes, a plugin setting default would be great, but still allow for grid specific override. Our client uses different grid patterns per Location.

@pszulczewski pszulczewski marked this pull request as ready for review March 22, 2024 19:02
@pszulczewski pszulczewski requested a review from joewesch March 22, 2024 19:02
@pszulczewski
Copy link
Contributor Author

What is the changelog in the CI? I am not sure why it fails?

@joewesch
Copy link
Contributor

What is the changelog in the CI? I am not sure why it fails?

This repo uses towncrier and requires a changelog fragment file: https://docs.nautobot.com/projects/floor-plan/en/latest/dev/contributing/#creating-changelog-fragments

@pszulczewski
Copy link
Contributor Author

pszulczewski commented Mar 25, 2024

What is the changelog in the CI? I am not sure why it fails?

This repo uses towncrier and requires a changelog fragment file: https://docs.nautobot.com/projects/floor-plan/en/latest/dev/contributing/#creating-changelog-fragments

Thanks. Looks like my branch was checked out beginning of February and it didn't have changes directory.
I've just re-based on develop and added a change file.

changes/79.added Outdated Show resolved Hide resolved
nautobot_floor_plan/utils.py Outdated Show resolved Hide resolved
docs/admin/install.md Outdated Show resolved Hide resolved
nautobot_floor_plan/models.py Outdated Show resolved Hide resolved
nautobot_floor_plan/models.py Outdated Show resolved Hide resolved
docs/admin/install.md Outdated Show resolved Hide resolved
@joewesch
Copy link
Contributor

joewesch commented Mar 25, 2024

Patryk, something isn't working right with the edit form. If I set one (or both) of the labels to letters and then edit the floor plan, the form field defaults back to numbers.

@joewesch
Copy link
Contributor

When setting a label to use letters, but then putting a number in the form it raise an inconsistent error message:
Screenshot 2024-03-25 at 12 14 50 PM

As part of the clean method we should do one of the following:

  • Check for an inconsistent type and raise the appropriate error
  • Automatically cast the number or letter to the proper value (my preferred option - save people from their own mistakes)

@pszulczewski
Copy link
Contributor Author

Let me check that.

@pszulczewski
Copy link
Contributor Author

pszulczewski commented Mar 25, 2024

Patryk, something isn't working right with the edit form. If I set one (or both) of the labels to letters and then edit the floor plan, the form field defaults back to numbers.

Looks like it stopped working after changing form field to CharField.
I fixed that and added validations. Should be good now, can you please pull and re-test?

pszulczewski and others added 4 commits March 25, 2024 20:23
Update from code review

Co-authored-by: Joe Wesch <10467633+joewesch@users.noreply.github.com>
@pszulczewski
Copy link
Contributor Author

@joewesch I have added unittests for the FloorPlanTileForm because there weren't any for that form, and they revealed that there were still some issues. I've made the necessary updates in forms, we should be good now.

docs/admin/install.md Outdated Show resolved Hide resolved
nautobot_floor_plan/forms.py Outdated Show resolved Hide resolved
Copy link
Contributor

@joewesch joewesch left a comment

Choose a reason for hiding this comment

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

Looks and works great now, thanks Patryk!

@joewesch
Copy link
Contributor

@glennmatthews would you mind giving this a once over?

docs/admin/install.md Outdated Show resolved Hide resolved
docs/admin/install.md Show resolved Hide resolved
nautobot_floor_plan/forms.py Show resolved Hide resolved
nautobot_floor_plan/forms.py Outdated Show resolved Hide resolved
Comment on lines +143 to +144
if self.letter_validator(field_name, value, axis) is not True:
return 0 # required to pass model clean() method
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you elaborate on this? I'd think that if the validator fails we should fail model clean?

Copy link
Contributor Author

@pszulczewski pszulczewski Apr 4, 2024

Choose a reason for hiding this comment

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

I was thinking to raise ValidationError in here and I was expecting that it will stop here, but what happens is that ValidationError exception is raised silently and it still goes to the model .clean() method, where it causes exception on the > operator TypeError: '>' not supported between instances of 'str' and 'int', what results with an exception page in the UI.

@joewesch joewesch merged commit 0755d00 into develop Apr 11, 2024
16 checks passed
@joewesch joewesch deleted the issue-8 branch April 11, 2024 19:40
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.

4 participants