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

Telecom - Carrier SIP Trunk #1382

Merged
merged 3 commits into from
Oct 14, 2019
Merged

Telecom - Carrier SIP Trunk #1382

merged 3 commits into from
Oct 14, 2019

Conversation

antleblanc
Copy link
Member

@antleblanc antleblanc commented Sep 30, 2019

Carrier SIP Trunk

This feature lives into the Telecom Control Panel.

✨ Feature

ac07cbc - feat(carrier.sip): init module
f6ac48e - feat(carrier.sip): init application
588756d - feat(telephony.carrier.sip): add new module

🔗 Related

🤝 Contributors

🏠 Internal

ref: MANAGER-3210

packages/manager/apps/carrier-sip/src/routing.js Outdated Show resolved Hide resolved
packages/manager/apps/telecom/webpack.config.js Outdated Show resolved Hide resolved
@import '~bootstrap4/scss/_grid';

.oui-page-header {
margin: 0 !important;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be a more specific selector if !important is needed

Copy link
Member Author

@antleblanc antleblanc Oct 1, 2019

Choose a reason for hiding this comment

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

The actual one is linked to the component name (eg: carrier-sip-dashboard .oui-page-header).

Could you tell what you had in mind to tackle the !important flag? Because I'm not sure to understand which selector we can used here.

Actual selector that affect us is (from @ovh-ux/manager-telecom-styles):

:not(.widget-presentation) {
  header:not(.widget-presentation-header):not(.page-header) {

packages/manager/modules/carrier-sip/src/index.js Outdated Show resolved Hide resolved
@antleblanc antleblanc changed the title Feat/carrier sip Telecom - Carrier SIP Trunk Oct 1, 2019
@antleblanc antleblanc requested a review from marie-j October 1, 2019 10:10
@antleblanc
Copy link
Member Author

@marie-j Thanks! I've applied changes regarding your comments.
Feel free to tell me if that's looks good for you.

Regarding the comment for using Iceberg, I let @JeremyDec answer it.

Thanks!

@JeremyDec JeremyDec force-pushed the feat/carrier-sip branch 3 times, most recently from 6b1e64b to 85fe745 Compare October 1, 2019 15:07
@@ -0,0 +1,30 @@
import angular from 'angular';

// Peer dependencies.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think comments should be used to say why you do something, not what you do, what do you think ? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added them to keep organized as much as possible in all imports.

Nothing blocker but yes we can find maybe a way to harmonize all imports.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, comments to organize our code is still a nice things. It's not like it's comments that explain nothing like :

// Check if true
if (foo === true) {
}

Copy link
Contributor

@FredericEspiau FredericEspiau Oct 8, 2019

Choose a reason for hiding this comment

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

If we need the same comments on 100 files, it probably means we need another solution than comments, like a coding convention :)

Or a tool that sort by itself :)

@antleblanc antleblanc force-pushed the feat/carrier-sip branch 2 times, most recently from 1d01dac to c2d4efe Compare October 2, 2019 09:02
@JeremyDec JeremyDec force-pushed the feat/carrier-sip branch 2 times, most recently from 5c785ee to 1a4bbcd Compare October 2, 2019 09:38
yarn.lock Outdated Show resolved Hide resolved
@antleblanc antleblanc force-pushed the feat/carrier-sip branch 2 times, most recently from 81e8db8 to c542e1e Compare October 3, 2019 08:22
@antleblanc antleblanc requested a review from marie-j October 3, 2019 08:22
@antleblanc antleblanc mentioned this pull request Oct 4, 2019
7 tasks
@antleblanc
Copy link
Member Author

Rebased on develop branch due to #1416.

AxelPeter
AxelPeter previously approved these changes Oct 8, 2019
FredericEspiau
FredericEspiau previously approved these changes Oct 8, 2019
Copy link
Contributor

@FredericEspiau FredericEspiau left a comment

Choose a reason for hiding this comment

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

Good job 👍

@@ -0,0 +1,8 @@
export const momentConfiguration = /* @ngInject */ (TranslateServiceProvider) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be done by the Core ?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, Nope.
The core module provides a service to get the user locale, his goal is not to configure all dependencies that are used by some modules (even though moment is frequently used).

@@ -0,0 +1,11 @@
{
"carrier_sip_cdr_title": "CDR",
"carrier_sip_cdr_information": "Le Call Details Records (CDR) contient toutes les informations relatives à un appel téléphonique : numéro de téléphone, durée d’appel, date et heure de l’appel, etc. Obtenez le détail des appels passés en téléchargeant le fichier .csv après avoir sélectionné la période souhaitée (mois et année) ci-dessous.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"carrier_sip_cdr_information": "Le Call Details Records (CDR) contient toutes les informations relatives à un appel téléphonique : numéro de téléphone, durée d’appel, date et heure de l’appel, etc. Obtenez le détail des appels passés en téléchargeant le fichier .csv après avoir sélectionné la période souhaitée (mois et année) ci-dessous.",
"carrier_sip_cdr_information": "Le Call Details Records (CDR) contient toutes les informations relatives à un appel téléphonique : numéro de téléphone, durée d’appel, date et heure de l’appel, etc. Obtenez le détail des appels passés en téléchargeant le fichier .csv après avoir sélectionné la période souhaitée (mois et année) ci-dessous.",

insecable space 😅 don't worry about it

antleblanc and others added 3 commits October 14, 2019 10:10
ref: MANAGER-3210

Co-authored-by: Marie JONES <[email protected]>
Co-authored-by: JeremyDec <[email protected]>

BREAKING CHANGE: init `@ovh-ux/manager-carrier-sip` module.
ref: MANAGER-3210

Co-authored-by: Marie JONES <[email protected]>
Co-authored-by: JeremyDec <[email protected]>

BREAKING CHANGE: init `@ovh-ux/manager-carrier-sip-app` application.
ref: MANAGER-3210

Co-authored-by: Marie JONES <[email protected]>
Co-authored-by: JeremyDec <[email protected]>
@antleblanc antleblanc merged commit 66bcfad into develop Oct 14, 2019
@antleblanc antleblanc deleted the feat/carrier-sip branch October 14, 2019 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants