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

DSP-759 Check if a Given Date Can Be Edited / Rename ValueTypeService to ValueService #214

Merged
merged 12 commits into from
Nov 2, 2020

Conversation

tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Oct 29, 2020

resolves DSP-759

Note that this PR renames ValueTypeService to ValueService.

@tobiasschweizer tobiasschweizer changed the title Check if a Given Date can be Edited Check if a Given Date Can Be Edited Oct 29, 2020
@tobiasschweizer tobiasschweizer self-assigned this Oct 29, 2020
@tobiasschweizer tobiasschweizer added the enhancement New feature or request label Oct 29, 2020
@tobiasschweizer
Copy link
Contributor Author

@mdelez This PR handles precision and era, but not the calendar.

@tobiasschweizer
Copy link
Contributor Author

tobiasschweizer commented Oct 29, 2020

@mdelez sorry, too early to review: still missing the logic the prevents the editing button to show up (isReadonly)

@kilchenmann kilchenmann changed the base branch from master to main October 29, 2020 16:50
@kilchenmann kilchenmann changed the title Check if a Given Date Can Be Edited DSP-759 Check if a Given Date Can Be Edited Oct 29, 2020
Tobias Schweizer added 2 commits October 30, 2020 08:39
# Conflicts:
#	projects/dsp-ui/src/lib/viewer/values/date-value/date-value.component.ts
@tobiasschweizer
Copy link
Contributor Author

@mdelez Ok, now this PR is ready for review :-)

I decided to move the logic to value type service since it is also used there. It seemed weird to me to use logic coming from a component in a service.

isTextEditable(textValue: ReadTextValueAsXml): boolean {
return textValue.mapping === 'http://rdfh.ch/standoff/mappings/StandardMapping';
}

/**
* Equality checks with constants below are TEMPORARY until component is implemented.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mdelez Maybe this comment should be adapted since there are also other reasons to make a value read-only. What do you think?

Copy link
Contributor

@mdelez mdelez Oct 30, 2020

Choose a reason for hiding this comment

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

I agree, it's strayed away from its initial purpose so the comment should be updated. Maybe something like "Determines if the editing the given value is supported"

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this service can also be renamed? It's becoming more general and is now more of a place to add helpful functions for the value components

Copy link
Contributor

@mdelez mdelez left a comment

Choose a reason for hiding this comment

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

Nice! Just left a few comments to discuss

isTextEditable(textValue: ReadTextValueAsXml): boolean {
return textValue.mapping === 'http://rdfh.ch/standoff/mappings/StandardMapping';
}

/**
* Equality checks with constants below are TEMPORARY until component is implemented.
Copy link
Contributor

@mdelez mdelez Oct 30, 2020

Choose a reason for hiding this comment

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

I agree, it's strayed away from its initial purpose so the comment should be updated. Maybe something like "Determines if the editing the given value is supported"

isTextEditable(textValue: ReadTextValueAsXml): boolean {
return textValue.mapping === 'http://rdfh.ch/standoff/mappings/StandardMapping';
}

/**
* Equality checks with constants below are TEMPORARY until component is implemented.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this service can also be renamed? It's becoming more general and is now more of a place to add helpful functions for the value components

@tobiasschweizer
Copy link
Contributor Author

Nice! Just left a few comments to discuss

I adapted the comment and renamed the service in b918f64.

I renamed the service to ValueService because it is not only about the value's type (the value instance is checked too).

Do you know if this service is used directly somewhere (outside the lib)? If yes, usage would break with the next release.

@mdelez
Copy link
Contributor

mdelez commented Nov 2, 2020

Do you know if this service is used directly somewhere (outside the lib)? If yes, usage would break with the next release.

It is used here in DSP-APP. However, the PR that this code is in isn't merged yet so we can update it easily :)

@tobiasschweizer
Copy link
Contributor Author

Do you know if this service is used directly somewhere (outside the lib)? If yes, usage would break with the next release.

It is used here in DSP-APP. However, the PR that this code is in isn't merged yet so we can update it easily :)

Ok, great! I adapted the vars as well in 3389906

@tobiasschweizer tobiasschweizer added the breaking Breaking change label Nov 2, 2020
@@ -324,14 +322,11 @@ describe('DisplayEditComponent', () => {
{
provide: MatDialogRef,
useValue: {}
},
ValueService,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mdelez I think there is no need to provide the service here since it is provided in root anyway.
Importing ReactiveFormsModule fixes the injector problem with FormBuilder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, I am not really sure what the best way is. Maybe we should spend some time on https://dasch.myjetbrains.com/youtrack/issue/DSP-98

Copy link
Contributor

@mdelez mdelez Nov 2, 2020

Choose a reason for hiding this comment

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

I'm currently writing unit tests for the PR mentioned above and the imports/injections (in test host components) for the FormBuilder are quite confusing. I think we should spend some time on what exactly needs to be imported in the unit tests so that you don't get an injector error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is an excellent idea. Maybe we could try to work on https://dasch.myjetbrains.com/youtrack/issue/DSP-98 soon.

@mdelez mdelez self-requested a review November 2, 2020 08:49
@tobiasschweizer tobiasschweizer merged commit d0db068 into main Nov 2, 2020
@tobiasschweizer tobiasschweizer deleted the wip/dsp-759-edit-date branch November 2, 2020 08:59
RalfBarkow pushed a commit that referenced this pull request Nov 4, 2020
* main:
  Update dependencies (#219)
  Create user service (#217)
  DSP-759 Check if a Given Date Can Be Edited (#214)
  DSP-920 Renaming default github branch to "main" (#216)
  DSP-881 - Value Component Base Class Changes (#210)
  chore (package.json): update dsp-js-lib and adapt assertions (#215)
  DSP-522 Refactor search panel style (#206)
  Wip/dsp 885 default gravsearch query (#211)

# Conflicts:
#	Makefile

( DSP-987 ) Still a bug in expert search
( DSP-885 ) Expert search default/example Gravsearch query throws a 400 Bad Request error
@kilchenmann kilchenmann changed the title DSP-759 Check if a Given Date Can Be Edited DSP-759 Check if a Given Date Can Be Edited / renames ValueTypeService to ValueService Dec 11, 2020
@kilchenmann kilchenmann changed the title DSP-759 Check if a Given Date Can Be Edited / renames ValueTypeService to ValueService DSP-759 Check if a Given Date Can Be Edited / Rename ValueTypeService to ValueService Dec 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants