plone / volto

React-based frontend for the Plone Content Management System
https://demo.plone.org/
MIT License
426 stars 575 forks source link

Folder content style #1343

Closed iFlameing closed 4 years ago

iFlameing commented 4 years ago

Fixes #1328. Styling the folder content as Pastanaga style.

sneridagh commented 4 years ago
Screenshot 2020-04-07 at 12 28 27
tisto commented 4 years ago

@iFlameing you have a merge conflict. Pls fix. :)

tisto commented 4 years ago

@iFlameing awesome work! I found a few things we could amend to even further improve things. This is really nitpicking. Though, we want to be as close as possible to Albert's ideas and design. :)

Contents (1)

Contents (2)

tisto commented 4 years ago

@iFlameing not really related to this PR and a know issue. Though, maybe you can look into this as well:

Contents (3)

tisto commented 4 years ago

@iFlameing thanks for the fixes. The icons next to the text are still off a little bit everywhere. Maybe that's something that @sneridagh needs to have a look.

tisto commented 4 years ago

@sneridagh @iFlameing we have to discuss what we want to show for content objects that do not have their own workflow state but inherit from the parent container (e.g. images and files). IMHO they should show the same "No workflow state" that we show in the toolbar menu with a grey dot. Maybe with a tooltip "This content object has no own workflow state but inherits the workflow state from the content object it is contained in".

Contents

tisto commented 4 years ago

@iFlameing paste icon in the folder_contents toolbar still has a grey background on hover. pls remove.

iFlameing commented 4 years ago

@tisto fixed except date :)

tisto commented 4 years ago

@sneridagh I just tried a git pull on this PR branch (locally) and now I have merge conflicts. I guess due to the force-push. TBH I don't get why ppl do force-push on git. Any idea why this kind of became a pattern? git force-push is dangerous in my opinion as a pattern and it leads to problems when multiple people work on a branch or even when someone just checked out the branch to try out things. IMHO we should discourage force-push altogether as an anti-pattern. Though, maybe there is a good reason for this approach that I don't see...

tisto commented 4 years ago

@iFlameing the Travis build is broken due to:

/home/travis/build/plone/volto/src/components/index.js
  97:8  error  Parsing error: `ContentsIndexHeader` has already been exported. Exported identifiers must be unique.
   95 | export SearchTags from '@plone/volto/components/theme/Search/SearchTags';
   96 | export CommentEditModal from '@plone/volto/components/theme/Comments/CommentEditModal';
>  97 | export ContentsIndexHeader from '@plone/volto/components/manage/Contents/ContentsIndexHeader';
      |        ^
   98 | export ContentsItem from '@plone/volto/components/manage/Contents/ContentsItem';
   99 | export ContentsUploadModal from '@plone/volto/components/manage/Contents/ContentsUploadModal';
  100 | export ContentsPropertiesModal from '@plone/volto/components/manage/Contents/ContentsPropertiesModal';
sneridagh commented 4 years ago

Yesterday I did a rearrangement of imports on the index.js file, I guess it’s due a bad merge. Reset to the master version then add local changes (if any) will do.

iFlameing commented 4 years ago

@sneridagh @tisto Yesterday, I did the rebase and accepted both changes. I will fix it.

iFlameing commented 4 years ago

@tisto can you check it now. I fixed all the test and resolve the conflict. Travis ci is falling due to not receiving any output in 10min.

tisto commented 4 years ago

@iFlameing will do! Don't worry about Travis for now. This is a problem we have on master as well right now...

tisto commented 4 years ago

@iFlameing the grey hover background on the upload icon is back and the three dots (select columns to show) are displaced again.

tisto commented 4 years ago

@iFlameing the icons and the labels should use the same baseline:

Screenshot 2020-04-11 at 14 09 02
iFlameing commented 4 years ago

@tisto in my local development the three dots are at the same position where It should be :) and there is no background image for upload icon on hover. Screenshot from 2020-04-11 17-52-59

tisto commented 4 years ago

@iFlameing odd. I did a fresh checkout...will check my env again and rebase master.

tisto commented 4 years ago

@iFlameing @sneridagh when we fix the line-height issue with the icons in the menu, I think we have something that we can merge and release. There are always details but we can improve those in an additional PR or later.

iFlameing commented 4 years ago

@tisto I think that this pr is mergable. I have an idea to resolve field but it is totally hardcoded like giving style to each individual icon. But this is not great solution. Can you merge this for now and then we can talk about this on Tuesday.

sneridagh commented 4 years ago

I would like to give it some more love, nothing important, small touches. I will look into it today.

On Sun, 12 Apr 2020 at 11:58, Alok Kumar notifications@github.com wrote:

@tisto https://github.com/tisto I think that this pr is mergable. I have an idea to resolve field but it is totally hardcoded like giving style to each individual icon. But this is not great solution. Can you merge this for now and then we can talk about this on Tuesday.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/plone/volto/pull/1343#issuecomment-612590770, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADW4D2EF3XZ53RR5W3J5O3RMGGDNANCNFSM4MC7DAGQ .

-- Víctor Fernández de Alba Github/Twitter/IRC: sneridagh

tisto commented 4 years ago

@iFlameing todo

-> after that we merge

iFlameing commented 4 years ago

@sneridagh Please don't merge it currently because I left it.only in cypress test(!!!! Sorry) and it is running only one test and that was passing. But due to recent changes, some test are broken and I am working on it. Some of the recent changes are choosing a date from a date picker and disabling the paste icon.

sneridagh commented 4 years ago

@iFlameing Green! Should we merge? :)

iFlameing commented 4 years ago

yes we can merge :)