mgr/dashboard: unify button/URL actions naming #26572
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
votdev
reviewed
Feb 22, 2019
| @@ -14,7 +14,7 @@ describe('PoolDetailsComponent', () => { | |||
|
|
|||
| configureTestBed({ | |||
| imports: [TabsModule.forRoot(), AppModule], | |||
| decalarations: [PoolDetailsComponent], | |||
| declarations: [PoolDetailsComponent], | |||
epuertat
force-pushed
the
wip-37337-master
branch
3 times, most recently
from
14ab23d
to
7846f34
Compare
epuertat
commented
Mar 1, 2019
src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-form/rbd-form.component.html
Outdated
Show resolved
Hide resolved
epuertat
force-pushed
the
wip-37337-master
branch
from
7846f34
to
e16f68e
Compare
votdev
requested review from
p-se,
s0nea,
tspmelo,
Devp00l and
ricardoasmarques
and removed request for
s0nea and
p-se
epuertat
force-pushed
the
wip-37337-master
branch
from
e16f68e
to
097c953
Compare
|
Could you please rebase this PR, otherwise there is a merge odyssey when you pull this PR into master. |
Sure. Tried late yesterday but the amount of changes was... awful. |
epuertat
force-pushed
the
wip-37337-master
branch
from
097c953
to
8038820
Compare
Still have merge conflicts. |
epuertat
force-pushed
the
wip-37337-master
branch
from
8038820
to
e824035
Compare
Yeah, sorry, I just posted the rebased PR. That one you tried was unrelated to that. |
epuertat
force-pushed
the
wip-37337-master
branch
4 times, most recently
from
27caf9a
to
1576862
Compare
|
I'm not sure but I guess the modals in the RGW user form doesn't follow the new wording style. It should also be "Remove" on the tooltip that currently says "Delete".
|
epuertat
force-pushed
the
wip-37337-master
branch
from
1d4189b
to
debba4b
Compare
|
Sorry @epuertat, you seem to have forgotten "Capabilities". |
alfonsomthd
approved these changes
Apr 2, 2019
epuertat
force-pushed
the
wip-37337-master
branch
from
debba4b
to
68ea16e
Compare
@Devp00l Hehe, that was an intentional omission (but anyway subject to further discussion): my assumption here is that 'Capabilities' fall into the category of items that can be 'added' to another object rather than 'created' from scratch, as they basically consist of existing elements ( For example, when you 'create' an S3 Key, you're actually defining and storing a new instance of that, but when you 'create' a capability you are rather re-arranging things that already existed. |
epuertat
force-pushed
the
wip-37337-master
branch
from
68ea16e
to
a08a6f4
Compare
- Mappings (actually an Enum) created for actions (buttons and other UI elements) and URLs: ActionLabels and URLVerbs. - An alternative would be to fix/improve the current i18n-polyfill, which only works with literal strings (not even with 'const enums' which become literals after Typescript transpiling). - Additionally having a predefined file with some strings to translate (actions, verbs, etc) could improve on the 1st of the 2-stage i18n process (as extraction tool has a lot of limitations). - A corresponding ActionLabelsI18n service with translated labels (it's a service as I haven't found the way to either translate no-const strings (ngx-translate/AST parser failure) or get a static translator). - This services could/should be extended to cover all strings that are defined in static/globally scoped objects before any I18n provider has been initialized. - Breadcrumbs are not translated (neither were they before this change). This part remains untackled: using 'proxy' static objects and performing live translation could deal with the issue. - New URLBuilder service created (following a established pattern in the Java/.NET world) . This should avoid the need of messing with literal URLs and string composition/parsing, and while the front-end is not meant to be consumed by anyone, Angular does not provide any other way for the app to navigate between components, so the URLs are a de-facto interface contract. Unlike this approach is not flawless, it's easier to enforce, while issues coming from free-from strings are really hard to catch. - This could be further improved by using a router registry/dynamic routing. Most of the routes are trivial. - As a side effect of these changes, routing module has been refactored and some routes moved to their specific modules (pool, rbd, rgw), via loadChildren and routes.forChild() magic. Now the above mentioned components are lazy-loaded/pre-loaded (it means right after the main code is loaded). This should also decrease the loading time (though probably this is not biggest time eater here). - As now modules can be loaded multiple times, not only from App module by means of lazy loading, but also from other ones (as PoolModule loads BlockModule to get QoS widgets in Pool windows), now lazy loaded modules include 2 NgModules (one with imports: RouterModule.forChild(routes), meant for lazy-loading, and another without routes). - Caveat: Some parts might not be (fully) translated (NFS, iSCSI, mirroring), as there's been ongoing work on them and it's hard to keep up with the new code. These changes will be a waste of time if the new code does not take benefit from/adheres to it, so I'm still figuring out how to spread this (nothing really fancy to demo). Maybe adding some checks/harnessing to enforce the new naming convention (ideas greatly welcome here). Fixes: http://tracker.ceph.com/issues/37337 Signed-off-by: Ernesto Puerta <epuertat@redhat.com>
Fixes: http://tracker.ceph.com/issues/37337 Signed-off-by: Ernesto Puerta <epuertat@redhat.com>
epuertat
force-pushed
the
wip-37337-master
branch
from
a08a6f4
to
b8dc3b5
Compare
epuertat
added a commit
to rhcs-dashboard/ceph
that referenced
this pull request
Apr 5, 2019
After PR ceph#26572, when RGW is not configured, accessing /rgw drop-down (daemons, users or buckets) results in nothing apparently happening (not even an error). Under the curtains, what is happening is that the ModuleStatusGuard has redirected the route to the rgw/501, but as this route is now under parent rgw route handler, which sets CanActivateChild guards, this results in a new ModuleStatusGuard invokation, a subsequent failure and a new redirection to rgw/501. Several approaches could be taken here: - Remove error pages from lazy-loaded modules. Probably it does not make sense to have a 501 page per component. - Add some whitelist to avoid this kind of loop (e.g.: 501, or any error page). - Set a max number of redirections (cautionary measure). Fixes: https://tracker.ceph.com/issues/39125 Signed-off-by: Ernesto Puerta <epuertat@redhat.com>
3 tasks
|
|
epuertat
added a commit
to rhcs-dashboard/ceph
that referenced
this pull request
Apr 8, 2019
After PR ceph#26572, when RGW is not configured, accessing /rgw drop-down (daemons, users or buckets) results in nothing apparently happening (not even an error). Under the curtains, what is happening is that the ModuleStatusGuard has redirected the route to the rgw/501, but as this route is now under parent rgw route handler, which sets CanActivateChild guards, this results in a new ModuleStatusGuard invokation, a subsequent failure and a new redirection to rgw/501. Several approaches could be taken here: - Remove error pages from lazy-loaded modules. Probably it does not make sense to have a 501 page per component. - Add some whitelist to avoid this kind of loop (e.g.: 501, or any error page). - Set a max number of redirections (cautionary measure). Fixes: https://tracker.ceph.com/issues/39125 Signed-off-by: Ernesto Puerta <epuertat@redhat.com>
epuertat
added a commit
to rhcs-dashboard/ceph
that referenced
this pull request
Apr 8, 2019
After PR ceph#26572, when RGW is not configured, accessing /rgw drop-down (daemons, users or buckets) results in nothing apparently happening (not even an error). Under the curtains, what is happening is that the ModuleStatusGuard has redirected the route to the rgw/501, but as this route is now under parent rgw route handler, which sets CanActivateChild guards, this results in a new ModuleStatusGuard invokation, a subsequent failure and a new redirection to rgw/501. Several approaches could be taken here: - Remove error pages from lazy-loaded modules. Probably it does not make sense to have a 501 page per component. - Add some whitelist to avoid this kind of loop (e.g.: 501, or any error page). - Set a max number of redirections (cautionary measure). Fixes: https://tracker.ceph.com/issues/39125 Signed-off-by: Ernesto Puerta <epuertat@redhat.com>
epuertat
added a commit
to rhcs-dashboard/ceph
that referenced
this pull request
Apr 8, 2019
After PR ceph#26572, when RGW is not configured, accessing /rgw drop-down (daemons, users or buckets) results in nothing apparently happening (not even an error). Under the curtains, what is happening is that the ModuleStatusGuard has redirected the route to the rgw/501, but as this route is now under parent rgw route handler, which sets CanActivateChild guards, this results in a new ModuleStatusGuard invokation, a subsequent failure and a new redirection to rgw/501. Several approaches could be taken here: - Remove error pages from lazy-loaded modules. Probably it does not make sense to have a 501 page per component. - Add some whitelist to avoid this kind of loop (e.g.: 501, or any error page). - Set a max number of redirections (cautionary measure). Fixes: https://tracker.ceph.com/issues/39125 Signed-off-by: Ernesto Puerta <epuertat@redhat.com>
epuertat
added a commit
to rhcs-dashboard/ceph
that referenced
this pull request
Apr 9, 2019
After PR ceph#26572, when RGW is not configured, accessing /rgw drop-down (daemons, users or buckets) results in nothing apparently happening (not even an error). Under the curtains, what is happening is that the ModuleStatusGuard has redirected the route to the rgw/501, but as this route is now under parent rgw route handler, which sets CanActivateChild guards, this results in a new ModuleStatusGuard invokation, a subsequent failure and a new redirection to rgw/501. Several approaches could be taken here: - Remove error pages from lazy-loaded modules. Probably it does not make sense to have a 501 page per component. - Add some whitelist to avoid this kind of loop (e.g.: 501, or any error page). - Set a max number of redirections (cautionary measure). Fixes: https://tracker.ceph.com/issues/39125 Signed-off-by: Ernesto Puerta <epuertat@redhat.com>
epuertat
added a commit
to rhcs-dashboard/ceph
that referenced
this pull request
Apr 9, 2019
After PR ceph#26572, when RGW is not configured, accessing /rgw drop-down (daemons, users or buckets) results in nothing apparently happening (not even an error). Under the curtains, what is happening is that the ModuleStatusGuard has redirected the route to the rgw/501, but as this route is now under parent rgw route handler, which sets CanActivateChild guards, this results in a new ModuleStatusGuard invokation, a subsequent failure and a new redirection to rgw/501. Several approaches could be taken here: - Remove error pages from lazy-loaded modules. Probably it does not make sense to have a 501 page per component. - Add some whitelist to avoid this kind of loop (e.g.: 501, or any error page). - Set a max number of redirections (cautionary measure). Fixes: https://tracker.ceph.com/issues/39125 Signed-off-by: Ernesto Puerta <epuertat@redhat.com>
epuertat
added a commit
to rhcs-dashboard/ceph
that referenced
this pull request
Apr 9, 2019
After PR ceph#26572, when RGW is not configured, accessing /rgw drop-down (daemons, users or buckets) results in nothing apparently happening (not even an error). Under the curtains, what is happening is that the ModuleStatusGuard has redirected the route to the rgw/501, but as this route is now under parent rgw route handler, which sets CanActivateChild guards, this results in a new ModuleStatusGuard invokation, a subsequent failure and a new redirection to rgw/501. Several approaches could be taken here: - Remove error pages from lazy-loaded modules. Probably it does not make sense to have a 501 page per component. - Add some whitelist to avoid this kind of loop (e.g.: 501, or any error page). - Set a max number of redirections (cautionary measure). Fixes: https://tracker.ceph.com/issues/39125 Signed-off-by: Ernesto Puerta <epuertat@redhat.com>
epuertat
added a commit
to rhcs-dashboard/ceph
that referenced
this pull request
Apr 10, 2019
After PR ceph#26572, when RGW is not configured, accessing /rgw drop-down (daemons, users or buckets) results in nothing apparently happening (not even an error). Under the curtains, what is happening is that the ModuleStatusGuard has redirected the route to the rgw/501, but as this route is now under parent rgw route handler, which sets CanActivateChild guards, this results in a new ModuleStatusGuard invokation, a subsequent failure and a new redirection to rgw/501. Several approaches could be taken here: - Remove error pages from lazy-loaded modules. Probably it does not make sense to have a 501 page per component. - Add some whitelist to avoid this kind of loop (e.g.: 501, or any error page). - Set a max number of redirections (cautionary measure). Fixes: https://tracker.ceph.com/issues/39125 Signed-off-by: Ernesto Puerta <epuertat@redhat.com> (cherry picked from commit c64b815) Signed-off-by: Ernesto Puerta <epuertat@redhat.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The screenshots below show an example of the changes performed. They've been applied to the top 8 components (Pools, RBD images, iSCSI, mirroring, RGW users, RGW buckets, Users and Roles).
Before:
After:
Detailed changes:
ActionLabelsandURLVerbs.ActionLabelsI18nservice with translated labels (it's a service as I haven't found the way to either translate no-const strings (ngx-translate/AST parser failure) or get a static translator).URLBuilderservice created (following a established pattern in the Java/.NET world) . This should avoid the need of messing with literal URLs and string composition/parsing, and while the front-end is not meant to be consumed by anyone, Angular does not provide any other way for the app to navigate between components, so the URLs are a de-facto interface contract. Unlike this approach is not flawless, it's easier to enforce, while issues coming from free-from strings are really hard to catch.loadChildrenandroutes.forChild()magic. Now the above mentioned components are lazy-loaded/pre-loaded (it means right after the main code is loaded). This should also decrease the loading time (though probably this is not biggest time eater here).NgModules(one withimports: RouterModule.forChild(routes), meant for lazy-loading, and another without routes).Fixes: http://tracker.ceph.com/issues/37337
Signed-off-by: Ernesto Puerta epuertat@redhat.com