Skip to content
This repository was archived by the owner on Jan 6, 2025. It is now read-only.

Update docs#1

Merged
Ks89 merged 2 commits intoKs89:masterfrom
luca-peruzzo:master
Aug 6, 2023
Merged

Update docs#1
Ks89 merged 2 commits intoKs89:masterfrom
luca-peruzzo:master

Conversation

@luca-peruzzo
Copy link
Copy Markdown
Contributor

@Ks89 as requested, I have updated docs and examples.
PS: The repo is broken because we need to update the main repo before

Luca Peruzzo added 2 commits July 17, 2023 13:35
adding new APIs and examples
<td>Font Awesome</td>
<td>IE11 support</td>
<td>Microsoft Edge (pre-chromium) support</td>
<td>LCP features</td>
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

probably, this is too much, because I cannot add a column for every new feature.
I created this table to show dependencies and browsers, not for specific features.

Comment thread src/app/shared/images.ts
{
img: `${PATH}/assets/images/carousel/milan-pegasus-gallery-statue.jpg`,
description: 'Description 1'
description: "Description 1",
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

it seems that you are using an IDE to replace quotes and add ",".
I'm always using a single quote ' to be consistent

Comment thread src/app/pages/features/common/common.html
Comment thread src/app/pages/features/common/common.html
</table>
<br>

<h2 id="Source" class="link clickable">
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

in my opinion, this interface should be moved inside features/modal-gallery because referenced only there.
Also, it's defined in ModalImage object, and not in a common one:

export interface ModalImage extends ImageData {
    extUrl?: string;
    downloadFileName?: string;
    sources?: Source[];
}

What do you think? Am I missing something?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have nothing against it

Comment thread src/app/pages/features/common/common.html
<td>No</td>
</tr>
<tr>
<td><b>11.1.0 (recommended)</b></td>
Copy link
Copy Markdown
Owner

@Ks89 Ks89 Jul 21, 2023

Choose a reason for hiding this comment

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

Also, I'm adding only the latest supported version for every major release.
I have to update previous rows with

  • 8.0.1
  • 9.1.0
  • 10.0.1
    because I missed this details in previous updates.

<h2 class="link clickable">
<a class="link-title" href="https://github.com/Ks89/angular-modal-gallery">Carousel lcp</a>
</h2>
<p>Carousel lcp demo.</p>
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Example is ok, but I think It needs some more information. For example, what does it mean LCP? I Suggest to write at least a small phrase with a link to a website with some official information about the topic.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we can link that: https://web.dev/lcp/

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

good idea

@Ks89 Ks89 merged commit a576e87 into Ks89:master Aug 6, 2023
@Ks89
Copy link
Copy Markdown
Owner

Ks89 commented Aug 6, 2023

merged, but I'll apply some fixes on top of this PR

Ks89 added a commit that referenced this pull request Aug 6, 2023
Signed-off-by: Stefano Cappa <stefano.cappa.ks89@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants