Dashboard uploads: refactor uploads view #1778
Conversation
Pineirin
force-pushed
the
uploads_refactor
branch
2 times, most recently
from
964f5db to
87a2bbb
Compare
...ets/semantic-ui/js/invenio_app_rdm/user_dashboard/uploads_items/ComputerTabletUploadsItem.js
Outdated
Show resolved
Hide resolved
...ets/semantic-ui/js/invenio_app_rdm/user_dashboard/uploads_items/ComputerTabletUploadsItem.js
Show resolved
Hide resolved
...ets/semantic-ui/js/invenio_app_rdm/user_dashboard/uploads_items/ComputerTabletUploadsItem.js
Outdated
Show resolved
Hide resolved
...heme/assets/semantic-ui/js/invenio_app_rdm/user_dashboard/uploads_items/MobileUploadsItem.js
Outdated
Show resolved
Hide resolved
invenio_app_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/utils.js
Outdated
Show resolved
Hide resolved
invenio_app_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/utils.js
Outdated
Show resolved
Hide resolved
invenio_app_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/utils.js
Outdated
Show resolved
Hide resolved
|
LGTM, left smaller comments / suggestions. Good work! Views are looking good 👍 |
Pineirin
force-pushed
the
uploads_refactor
branch
2 times, most recently
from
d4aee10 to
6d27c71
Compare
...ets/semantic-ui/js/invenio_app_rdm/user_dashboard/uploads_items/ComputerTabletUploadsItem.js
Outdated
Show resolved
Hide resolved
...ets/semantic-ui/js/invenio_app_rdm/user_dashboard/uploads_items/ComputerTabletUploadsItem.js
Outdated
Show resolved
Hide resolved
| @@ -108,7 +106,7 @@ export function SearchItemCreators({ creators }) { | |||
| return link; | |||
| } | |||
| return creators.map((creator, index) => ( | |||
| <span className="creatibutor-wrap separated" key={index}> | |||
| <span className={spanClass} key={index}> | |||
There was a problem hiding this comment.
| <span className={spanClass} key={index}> | |
| <span className={`creatibutor-wrap separated ${className}`} key={index}> |
Can we keep "creatibutor-wrap separated" always by default, then we don't need to add these classes to the mobile view.
There was a problem hiding this comment.
I'm not sure if it would be best to allow the component to have all it's className overridden, as it would make it more reusable. I've made the change but I would like if others in the team give their opinion.
There was a problem hiding this comment.
if they should always have this class and will otherwise break the component ui then it makes sense to always pass it, otherwise i would prefer to make it completely overridable
There was a problem hiding this comment.
As far as I know, the classes belong to the component and should not be overridden. It is the name with connected identifier icon which we want to keep consistent in styling across the entire site.
|
|
||
| export const MobileUploadsItem = ({ result, index, editRecord, statuses }) => { | ||
| const access_status_id = _get( | ||
| result, |
There was a problem hiding this comment.
Could maybe all these shared variables be collected in the parent and passed down to the computer/mobile component, so that we don't need to define them twice?
There was a problem hiding this comment.
I agree with you, but I got some backlash in another similar PR about having components with a huge number of parameters passed. I would say we need to establish clearly what should and should not get duplicated @kpsherva.
There was a problem hiding this comment.
this could be an utility function, it is basically serializing the result object, so it could be a serializer. Just a suggestion.
Pineirin
force-pushed
the
uploads_refactor
branch
10 times, most recently
from
9fb3051 to
8a4eeca
Compare
| import { i18next } from "@translations/invenio_app_rdm/i18next"; | ||
| import _get from "lodash/get"; | ||
|
|
||
| export const uploadsResultProps = (result) => { |
There was a problem hiding this comment.
it's quite unorthodox in our code base to separate a function to provide the props
additionally, as a developer I wouldn't like to jump to another file to find out how my props are calculated.
most of these statements can be simplified, let me know if you need hints on how - please place them in the components in which they are used
...heme/assets/semantic-ui/js/invenio_app_rdm/user_dashboard/uploads_items/MobileUploadsItem.js
Outdated
Show resolved
Hide resolved
...ets/semantic-ui/js/invenio_app_rdm/user_dashboard/uploads_items/ComputerTabletUploadsItem.js
Outdated
Show resolved
Hide resolved
Pineirin
force-pushed
the
uploads_refactor
branch
2 times, most recently
from
be16f09 to
c671d06
Compare
Pineirin
force-pushed
the
uploads_refactor
branch
2 times, most recently
from
96bd36e to
9e10e00
Compare
* remove empty content. * create a custom mobile interface. * fix request_items naming. * closes: inveniosoftware#1771
Pineirin
force-pushed
the
uploads_refactor
branch
from
9e10e00 to
595a413
Compare
| </Label> | ||
| ))} | ||
| <div> | ||
| {} |
| onClick={() => editRecord()} | ||
| className="fluid-responsive" | ||
| > | ||
| <Icon name="edit" /> |
There was a problem hiding this comment.
maybe there is a difference in how it looks but otherwise you can also pass the icon prop to the button 😄 . Same as below
Remove empty content.
Create a custom mobile interface.
Fix request_items naming.
closes: #1771
Computer:
Tablet:
Mobile:
