Skip to content

Uniformation of Edit Properties and detail card#5834

Merged
offtherailz merged 5 commits intogeosolutions-it:masterfrom
vlt1:enhancement-5772
Sep 10, 2020
Merged

Uniformation of Edit Properties and detail card#5834
offtherailz merged 5 commits intogeosolutions-it:masterfrom
vlt1:enhancement-5772

Conversation

@vlt1
Copy link
Copy Markdown
Contributor

@vlt1 vlt1 commented Sep 3, 2020 •
edited by tdipisa
Loading

Description

Replaces MetadataModal with existing general SaveModal in MapGrid. Adds details sheet to that SaveModal. Also should solve this behaviour.

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe: enhancement

Issue

#5772

What is the current behavior?

#5772

What is the new behavior?
MetadataModal in MapGrid is removed and replaced with SaveModal. SaveModal is extended with details sheet.

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • Yes, and I documented them in migration notes
  • No

@vlt1 vlt1 added this to the 2020.03.00 milestone Sep 3, 2020
@vlt1 vlt1 self-assigned this Sep 3, 2020
@coveralls
Copy link
Copy Markdown

coveralls commented Sep 3, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.3%) to 83.259% when pulling d9dc1de on vlt1:enhancement-5772 into 3b0bf21 on geosolutions-it:master.

@tdipisa tdipisa modified the milestones: 2020.03.00, 2020.02.01 Sep 7, 2020
@tdipisa tdipisa requested a review from offtherailz September 7, 2020 07:58
offtherailz
offtherailz requested changes Sep 7, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@offtherailz offtherailz left a comment •
edited by vlt1
Loading

Choose a reason for hiding this comment

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

  • Great! We cleaned up a lot. I'd like to clean up even more. I think we can reuse almost everything from the ResourceGrid. Please look at my inline comments for details

  • I see there is an undo button when you create a new detail card that should not appear. Probably is due to the NODATA handling I mentioned
    image

  • If I remove from the FeatureGrid the detail card, I still see the detail button. Clicking on it causes shows a "NODATA"
    image

  • Please add unit tests for new things you wrote.

@vlt1
Copy link
Copy Markdown
Contributor Author

vlt1 commented Sep 7, 2020

@offtherailz when should the undo button appear

@offtherailz
Copy link
Copy Markdown
Member

offtherailz commented Sep 7, 2020

@vlt1 checking the current behaviour, only when the user deleted the detail card, to restore it. In fact I see the text is "undo remove"

@vlt1
Copy link
Copy Markdown
Contributor Author

vlt1 commented Sep 8, 2020

@offtherailz if rework is good I'll add tests

@vlt1 vlt1 requested a review from offtherailz September 9, 2020 13:53
@offtherailz
Copy link
Copy Markdown
Member

offtherailz commented Sep 10, 2020 •
edited
Loading

@vlt1 looks really great! 👍 🚀
Please move testing the new/uncovered parts (they should be not that much).
As usual tests that make sense the functionalities and corner cases of units. ;) We already increased a little the coverage just removing unuseful code.

https://coveralls.io/builds/33329679
image

@offtherailz offtherailz requested review from allyoucanmap and offtherailz and removed request for allyoucanmap and offtherailz September 10, 2020 09:46
@offtherailz offtherailz merged commit a1e39aa into geosolutions-it:master Sep 10, 2020
@allyoucanmap allyoucanmap mentioned this pull request Aug 9, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@offtherailz offtherailz offtherailz approved these changes

Assignees

@vlt1 vlt1

Labels

enhancement Priority: High

Projects

None yet

Milestone

2021.01.00

Development

Successfully merging this pull request may close these issues.

4 participants

@vlt1 @coveralls @offtherailz @tdipisa