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

DT v2 + Project structure upgrades #1783

Merged
merged 16 commits into from
Apr 21, 2024
Merged

DT v2 + Project structure upgrades #1783

merged 16 commits into from
Apr 21, 2024

Conversation

shanmukhateja
Copy link
Collaborator

Closes #1777 and #1780

Feel free to test this branch for issues. I have verified the functionality using npm test.

Also, I suggest cloning this branch in a separate directory due to differences in project structure and devDependencies.

@shanmukhateja shanmukhateja added enhancement dependencies Pull requests that update a dependency file chore Changes to groom the project labels Apr 6, 2024
@shanmukhateja shanmukhateja requested a review from l-lin April 6, 2024 18:28
@shanmukhateja
Copy link
Collaborator Author

shanmukhateja commented Apr 6, 2024

@l-lin I may have broken GitHub CI flow with these changes. We simply need to update npm commands used in the workflow file and test it back-to-back.

If you can spare some time, please look into this. It might take me a while to check this due to some personal commitments. :-^)

EDIT: CI updates have been made. Tested with act locally and they pass. I could always use more testing. :-)

- Update CI test code

- Tested using `act` but could use more testing
- This needs to be re-introduced in the future
@shanmukhateja
Copy link
Collaborator Author

I still need to update documentation pages and DEVELOPER.md

- update DEVELOPER.md

- CSS bug fixes in demo app on mobile screens

- remove spec file for lib

- update .gitignore, .npmignore

- update project deps
- remove unused paths in tsconfig.json
This looks nicer when viewing on a 15in laptop screen. Also, it lines up perfectly with the A+DT icon on the sidebar
Copy link
Owner

@l-lin l-lin left a comment

Choose a reason for hiding this comment

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

chore: We need another change on the documentation where we need to specify that angular-datatables will only support datatables v2 starting from version X.

lib/public_api.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
DEVELOPER.md Show resolved Hide resolved
demo/src/app/app.component.css Show resolved Hide resolved
@shanmukhateja
Copy link
Collaborator Author

Hey @l-lin

Glad to know you're back.

I have just closed the laptop for the night so let me check and update the PR tomorrow (IST timezone).

:)

@shanmukhateja
Copy link
Collaborator Author

shanmukhateja commented Apr 20, 2024

@l-lin Please don't merge the PR just yet. I need to test ng add with Bootstrap + DT v2.

EDIT: Bootstrap integration still works!

Feel free to merge when you approve :)

Copy link
Owner

@l-lin l-lin left a comment

Choose a reason for hiding this comment

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

chore: We need another change on the documentation where we need to specify that angular-datatables will only support datatables v2 starting from version X.

I was only expecting a small quote on the FAQ page. What you implemented is way better 👍

However, I think we need to invert the sources, i.e. md(HTML|TS) for source on DT v2 and have a md(HTML|TS)DTV1 for V1 instead.

Indeed, we will update and maintain the documentation for DT v2 and the ones for DT V1 will not be touched (or will be deleted in the future). In other words, we should use variable names with suffix/prefix for special cases and have the "standard" variable name for the documentation we need to keep up-to-date.

The benefit is double:

  • it's implicit that "standard" variable name like "mdHTML" is the main one
  • if we delete the documentation for legacy DT v1, it's easier to do it by searching the keyword "v1" (or legacy?)

What do you think?

- all dtv1 md files are suffixed `-dtv1.md`

- Deprecated menus are now hidden in v2 but the URLs will still work.
@shanmukhateja
Copy link
Collaborator Author

chore: We need another change on the documentation where we need to specify that angular-datatables will only support datatables v2 starting from version X.

I was only expecting a small quote on the FAQ page. What you implemented is way better 👍

However, I think we need to invert the sources, i.e. md(HTML|TS) for source on DT v2 and have a md(HTML|TS)DTV1 for V1 instead.

Indeed, we will update and maintain the documentation for DT v2 and the ones for DT V1 will not be touched (or will be deleted in the future). In other words, we should use variable names with suffix/prefix for special cases and have the "standard" variable name for the documentation we need to keep up-to-date.

The benefit is double:

* it's implicit that "standard" variable name like "mdHTML" is the main one

* if we delete the documentation for legacy DT v1, it's easier to do it by searching the keyword "v1" (or legacy?)

What do you think?

In hindsight, I should've done it this way.

Fixed in e473f9d (#1783)

@l-lin l-lin merged commit aa14f0c into master Apr 21, 2024
1 check passed
@l-lin l-lin deleted the dt-2 branch April 21, 2024 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Changes to groom the project dependencies Pull requests that update a dependency file enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants