Skip to content

Make create, edit community/collection/item dialogs theme-able.#1779

Merged
tdonohue merged 5 commits intoDSpace:mainfrom
mwoodiupui:themeable-dialogs
Sep 7, 2022
Merged

Make create, edit community/collection/item dialogs theme-able.#1779
tdonohue merged 5 commits intoDSpace:mainfrom
mwoodiupui:themeable-dialogs

Conversation

@mwoodiupui
Copy link
Copy Markdown
Member

@mwoodiupui mwoodiupui commented Aug 19, 2022

Description

See the title. This is an elaboration of local work that was needed to create a custom new-item dialog in our local theme (instead of hacking the stock dialog).

Instructions for Reviewers

List of changes in this PR:

  • Create themed wrappers for the dialog components.
  • Use them in the menu resolver, in place of the un-themed components.

Checklist

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes TSLint validation using yarn run lint
  • My PR doesn't introduce circular dependencies
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

@mwoodiupui mwoodiupui added improvement component: submission component: administrative tools Related to the admin menu or tools themes component: Community Community display or editing component: Collection Collection display or editing labels Aug 19, 2022
@mwoodiupui mwoodiupui self-assigned this Aug 19, 2022
@mwoodiupui
Copy link
Copy Markdown
Member Author

This is draft because I'm running Node v18 but the specified Webpack and eslint-plugin-jsdoc require an earlier version.

@mwoodiupui mwoodiupui marked this pull request as ready for review August 22, 2022 16:26
@tdonohue tdonohue added this to the 7.4 milestone Aug 22, 2022
@tdonohue tdonohue requested a review from artlowel August 25, 2022 14:52
@tdonohue tdonohue added the 1 APPROVAL pull request only requires a single approval to merge label Aug 25, 2022
Copy link
Copy Markdown
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

Thanks @mwoodiupui

What you've written here looks correct at first sight, but a few things are missing:

  • Those new ThemedComponents have to be declared in a module for them to work, the same module the components they're theming are declared in, I think that's SharedModule in this case
  • When theming a component you should provide an example themed version in the custom theme. See HeaderComponent in the custom theme for example That way, people that want to use your themed component have something to start from. You can use that version in the custom component yourself as well to verify that everything is wired up correctly. If you modify it, and use the custom theme, those modifications should show up

@mwoodiupui mwoodiupui requested a review from artlowel September 2, 2022 16:42
Copy link
Copy Markdown
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

Thanks @mwoodiupui

I have a few more comments to bring these in line with the other components in the custom theme. You picked an unconventional set of components to get started with (ones that share a single html template) which complicates things a bit

Comment on lines +2 to +9
import {
CreateCollectionParentSelectorComponent as BaseComponent
} from 'src/app/shared/dso-selector/modal-wrappers/create-collection-parent-selector/create-collection-parent-selector.component';

@Component({
selector: 'ds-create-collection-parent-selector',
templateUrl: '../dso-selector-modal-wrapper.component.html',
})
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.

The convention for the custom theme is to already create html and css file for the component, but add them in comments and refer back to the version in the base theme.

That means that if you enable the theme, everything looks exactly like the base theme. Then if you want to start theming the component all you need to do is swap the comments around and you can start overwriting the css or the html right away.

Also, I've noticed that using absolute paths in imports tends to cause build problems in certain scenarios that are hard to diagnose, so I'd suggest always using relative paths, even though that can lead to a ridiculous number of ../-es, like in this case. However most IDEs can keep count for you.

Suggested change
import {
CreateCollectionParentSelectorComponent as BaseComponent
} from 'src/app/shared/dso-selector/modal-wrappers/create-collection-parent-selector/create-collection-parent-selector.component';
@Component({
selector: 'ds-create-collection-parent-selector',
templateUrl: '../dso-selector-modal-wrapper.component.html',
})
import {
CreateCollectionParentSelectorComponent as BaseComponent
} from '../../../../../../../app/shared/dso-selector/modal-wrappers/create-collection-parent-selector/create-collection-parent-selector.component';
@Component({
selector: 'ds-create-collection-parent-selector',
// styleUrls: ['create-collection-parent-selector.component.scss'],
// templateUrl: 'create-collection-parent-selector.component.html',
templateUrl: '../../../../../../../app/shared/dso-selector/modal-wrappers/dso-selector-modal-wrapper.component.html',
})

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done, I think.

Comment on lines +1 to +11
<div>
<div class="modal-header">{{'dso-selector.'+ action + '.' + objectType.toString().toLowerCase() + '.head' | translate}}
<button type="button" class="close" (click)="close()" aria-label="Close">
<span aria-hidden="true">×</span>
</button>
</div>
<div class="modal-body">
<h5 *ngIf="header" class="px-2">{{header | translate}}</h5>
<ds-dso-selector [currentDSOId]="dsoRD?.payload.uuid" [types]="selectorTypes" (onSelect)="selectObject($event)"></ds-dso-selector>
</div>
</div>
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.

Rather than moving this file to the theme folder when it's not itself a themed component, I'd reference the version in the base theme as detailed in a comment above, and delete this file

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, this file becomes useless. Done.

@mwoodiupui
Copy link
Copy Markdown
Member Author

I have a knack for starting out to learn with a problem that seems simple but turns out to be difficult or unusual. Thanks for a thorough critique.

@mwoodiupui mwoodiupui requested a review from artlowel September 6, 2022 19:47
Copy link
Copy Markdown
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

No problem, thanks for incorporating it in your PR!

@tdonohue tdonohue merged commit e523583 into DSpace:main Sep 7, 2022
@mwoodiupui mwoodiupui deleted the themeable-dialogs branch September 7, 2022 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 APPROVAL pull request only requires a single approval to merge component: administrative tools Related to the admin menu or tools component: Collection Collection display or editing component: Community Community display or editing component: submission improvement themes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants