[PRODX-20593] Add new MCC Connection page #659
Conversation
bnalisnikovskiy
force-pushed
the
PRODX-20593_Add_AddCloudInstance
branch
from
75573c4
to
1c85d9d
Compare
stefcameron
changed the title
| @@ -7,7 +7,7 @@ import { ConfigProvider } from '../../store/ConfigProvider'; | |||
| import { SsoAuthProvider } from '../../store/SsoAuthProvider'; | |||
| import { ClusterDataProvider } from '../../store/ClusterDataProvider'; | |||
| import { ClusterActionsProvider } from '../../store/ClusterActionsProvider'; | |||
| import { lightThemeClassName, lightTheme, darkTheme } from '../theme'; | |||
| import { lightThemeClassName, lightTheme, darkTheme } from '../theme.js'; | |||
There was a problem hiding this comment.
Seems like a "random" IDE settings change to add the ".js" file extension. Those extensions aren't necessary, given the Webpack setup. Would be good to avoid adding it and keep the code consistent -- unless there's a good reason to add it that I'm not thinking of?
There was a problem hiding this comment.
No, it's me. My IDE (WebStrom from JetBrans) cannot recognize those imports. I mean, if file is .tsx then import without extension doesn't work (in IDE). Probably it tries fo find ts file as well. I didn't find a way to fix that and allow 'mixing' file types fro IDE
There was a problem hiding this comment.
It's strange that it only updated that specific import, and not all the other ones too, since by this logic, wouldn't they all exhibit the same issue in WebStorm?
There was a problem hiding this comment.
I meant that I did that in purpose. Because without file extension variables seem as unused inside their files. IDE just can't 'net' them. Later I'll try to find solution again...
There was a problem hiding this comment.
Strange. Doesn't WebStorm use ESLint (and our specific configuration of it) for linting the code? Seems like it's using its own linter with its own rules. Maybe that's a clue to fixing this.
There was a problem hiding this comment.
Please try to get this fix, but we won't let it block this PR.
There was a problem hiding this comment.
It uses Eslint as well, and have its own internal checks
bnalisnikovskiy
force-pushed
the
PRODX-20593_Add_AddCloudInstance
branch
from
1c85d9d
to
d17e147
Compare
|
I understand you're just adding UI and I guess not wanting to connect it to anything just yet, but my concern with the state setter and the hardcoded string is that this creates technical debt that now needs to be tracked somehow and cleaned-up. You know that's the case, but say you're away from work for some reason, and it's Anton that needs to do follow-up work, will he remember all these little details? Will I? We know we'll need the string, so why not add it now and be done with it? If the string isn't correct, it can be updated later. But at least we won't introduce a hardcoded string in the code we need to remember to fix later. We know we'll need the event props, so why not add them now (at least the I see these two small things as a way to prevent the accumulation of many small things here and there that could lead to a potentially big problem. Let's keep the code base as clean as possible at all times. The use of the state providers in the ConnectionBlock is of less concern since that feels more isolated and seems more obvious that it needs to be replaced, even if it wasn't you that did the follow-up work. |
|
Sure. It's not a big changes. Now I see what you meant. Fixed |
Thank you! 😄 |
bnalisnikovskiy
force-pushed
the
PRODX-20593_Add_AddCloudInstance
branch
from
a30438c
to
28a67e9
Compare
To go to new page do
shift+clickon SyncPage title.this is just a mockup. All other logic and connection with other parts will be added in the future tickets