2548: Refactoring #3012
2548: Refactoring #3012
Conversation
LeandraH
requested review from
steffenkleinle,
ztefanie and
f1sh1918
as code owners
| @@ -8,7 +8,7 @@ import config from 'translations/src/config' | |||
| import { PdfIcon } from '../assets' | |||
| import { cmsApiBaseUrl } from '../constants/urls' | |||
| import CityContentToolbar from './CityContentToolbar' | |||
| import ToolbarItem from './ToolbarItem' | |||
| import ToolbarIcon from './ToolbarIcon' | |||
There was a problem hiding this comment.
🤔 Hmm, I sadly don't like this new naming as it misses the point of what the component does. It is not just an icon, however, it is an item (or button). So please revert (to ToolbarItem or change to ToolbarItems if you want to keep the current behavior of having both items rendered in one component.
However, as mentioned in the first PR (#3004 (comment)), I'd prefer only having it render one toolbar item, see #3015 for a proposal on specifics. I think this would be a clearer separation of concerns and more intuitive on what a specific component does.
There was a problem hiding this comment.
You're right, that is clearer! Do you want to continue in your PR or should I take over?
There was a problem hiding this comment.
Go for it then, I'll close this one :)
Short description
As discussed in #3004 (comment), this just renames the component
FeedbackToolbarItemtoFeedbackToolbarIconsbecause -- as @steffenkleinle correctly pointed out -- it is confusing to have a component namedFeedbackToolbarItemreturning twoItems.Resolved issues
Fixes: #2548