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

New frontend UX design #246

Open
wants to merge 49 commits into
base: master
Choose a base branch
from

Conversation

Matthbo
Copy link
Member

@Matthbo Matthbo commented Dec 12, 2024

Based on the FF!Reference design guide & angular components library

frankdoc-zoek-pagina frankdoc-zoek-filteren frankdoc-detail-pagina

@Matthbo Matthbo self-assigned this Dec 12, 2024
@Matthbo Matthbo linked an issue Dec 12, 2024 that may be closed by this pull request
2 tasks
@Matthbo Matthbo marked this pull request as ready for review December 12, 2024 15:02
Comment on lines 24 to 56
collapseElement(): void {
this.collapse.style.overflowY = 'hidden';
this.collapse
.animate({ height: [`${this.collapse.clientHeight}px`, '0px'] }, this.animationSpeed)
.finished.then(() => {
this.collapse.style.height = '0px';
this.collapse.style.overflowY = '';
this.collapse.style.overflow = 'hidden';
});
}

expandElement(): void {
this.collapse.style.height = '';
this.collapse.style.overflowY = 'hidden';
this.collapse
.animate({ height: ['0px', `${this.collapse.clientHeight}px`] }, this.animationSpeed)
.finished.then(() => {
this.collapse.removeAttribute('style');
});
}
Copy link
Member

@philipsens philipsens Dec 13, 2024

Choose a reason for hiding this comment

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

Why this way instead of adding and removing classes?
classList.add('collapsed') -> classList.remove('collapsed')

Is it possible to cancel the animation if it is uncollapsed/collapsed rapidly?

</nav>
<div class="actions">
<button class="switch-theme-button" title="Dark/light Mode">
<app-icon-darkmode width="20" height="20" colour="#8A8A8A"></app-icon-darkmode>
Copy link
Member

@philipsens philipsens Dec 13, 2024

Choose a reason for hiding this comment

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

It might be smart to create vars for colors, to make then reusable and easy to change.
This would also apply to the colors in the CSS itself.

Comment on lines 22 to 28
private templateRef: TemplateRef<TemplateContext> = inject(TemplateRef);
private viewContainerRef: ViewContainerRef = inject(ViewContainerRef);
private appService: AppService = inject(AppService);

private markdownLinkRegex = /\[(.*?)]\((.+?)\)/g; // old regex: /\[(.*?)\]\((.+?)\)/g
private tagsRegex = /<[^>]*>?/gm;
private linkRegex = /(?:{@link\s(.*?)})/g;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private templateRef: TemplateRef<TemplateContext> = inject(TemplateRef);
private viewContainerRef: ViewContainerRef = inject(ViewContainerRef);
private appService: AppService = inject(AppService);
private markdownLinkRegex = /\[(.*?)]\((.+?)\)/g; // old regex: /\[(.*?)\]\((.+?)\)/g
private tagsRegex = /<[^>]*>?/gm;
private linkRegex = /(?:{@link\s(.*?)})/g;
private readonly templateRef: TemplateRef<TemplateContext> = inject(TemplateRef);
private readonly viewContainerRef: ViewContainerRef = inject(ViewContainerRef);
private readonly appService: AppService = inject(AppService);
private readonly markdownLinkRegex = /\[(.*?)]\((.+?)\)/g; // old regex: /\[(.*?)\]\((.+?)\)/g
private readonly tagsRegex = /<[^>]*>?/gm;
private readonly linkRegex = /(?:{@link\s(.*?)})/g;

It could be good practice to make stuff like this readonly.

private generateElementSyntax(): void {
if (this.element) {
this.elementSyntax = `
<${this.element.name} name=”my-${this.element.name.toLowerCase()}” />
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<${this.element.name} name=my-${this.element.name.toLowerCase()} />
<${this.element.name} name="my-${this.element.name.toLowerCase()}" />

getFirstLabel(component.item.labels),
component.item.name,
]"
>-></ff-button
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
>-></ff-button
></ff-button

:D

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a nice character to use instead of 2, but according to another comment you would rather have an html entity, I assume. I found this &rarr; (→) as possible option but maybe you can find a better one?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, you are completely right. It might be best to even use a real icon for this.

>
</div>
} @empty {
<p>No Frank!Elements found.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Die naam D:

@for (selectedFilter of selectedFilterGroup.value; track selectedFilter; let index = $index) {
<span class="selected-filter"
><span>{{ selectedFilter }}</span
><span class="close-button" (click)="removeSelectedFilterLabel(selectedFilterGroup.key, index)">x</span></span
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
><span class="close-button" (click)="removeSelectedFilterLabel(selectedFilterGroup.key, index)">x</span></span
><span class="close-button" (click)="removeSelectedFilterLabel(selectedFilterGroup.key, index)">&times;</span></span

Maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is these a difference character x and &times;? I compared them and see no visual difference so what would be the benefit?

Copy link
Member

Choose a reason for hiding this comment

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

I thought the one looked more like a cross. (× vs x)

private checkbox: CheckboxComponent = inject(CheckboxComponent);

ngOnChanges(): void {
this.checkbox.checked = this.selectedFilterLabels[this.initFilterName]?.includes(this.initLabelName) ?? false;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.checkbox.checked = this.selectedFilterLabels[this.initFilterName]?.includes(this.initLabelName) ?? false;
this.checkbox.checked = !!this.selectedFilterLabels[this.initFilterName]?.includes(this.initLabelName);

<div class="section syntax">
<h3>Syntax</h3>
<pre><button (click)="copyToClipboard(elementSyntax)">Copy</button><code [innerText]="elementSyntax"
>&lt;{{element.name}} name=”my-test-pipe” version=”1” /&gt;</code
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
>&lt;{{element.name}} name=my-test-pipe version=”1” /&gt;</code
>&lt;{{element.name}} name="my-test-pipe" version="1" /&gt;</code

@Matthbo Matthbo force-pushed the 235-implement-new-frontend-design-frankdoc-format branch from cc6bdca to b3f7f28 Compare December 17, 2024 17:26
Comment on lines +85 to +94
if (!labels) return '-';
const labelGroups = Object.keys(labels);
if (labelGroups.length === 0) return '-';
return labelGroups[0];
}

copyOf<T>(
baseAttributes: T[] | undefined,
mergeAttributes: T[] | undefined,
fieldName: keyof T
): T[] | null {
if (baseAttributes && !mergeAttributes) return baseAttributes;
else if (mergeAttributes && !baseAttributes) return mergeAttributes;
else if (!baseAttributes && !mergeAttributes) return null;
getFirstLabel(labels?: Record<string, string[]>): string {
if (!labels) return '-';
const labelGroups = Object.keys(labels);
if (labelGroups.length === 0) return '-';
Copy link
Member

Choose a reason for hiding this comment

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

It might be clearer to what the '-' is if it was a constant.
if (!labels) return this.NO_RESULTS_CHARACTER; or something along those lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement new frontend design & frankdoc format
2 participants