Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Makes shift+return create a new line in a text tile #785

Closed
wants to merge 24 commits into from

Conversation

rodfersou
Copy link
Member

@rodfersou rodfersou commented Jun 22, 2019

  • Add shift+return for Text tile
  • Added cypress tests for every tile
    image

Those should be fixed on other PR:

  • The hero tile needs to implement react dropzone to be able to finish the test. #804
  • Table tile not working (at least for tests). #805
  • Tiles Image, Video and Maps don't work in Guilhotina. #806
  • Improve Text tile test. #807

@rodfersou rodfersou requested review from robgietema and sneridagh Jun 22, 2019
@tisto
Copy link
Member

@tisto tisto commented Jun 22, 2019

@rodfersou the new cypress test fails.

@robgietema
Copy link
Member

@robgietema robgietema commented Jun 23, 2019

The description is saved as a string right? and rendered HTML, so the newline will not be visible. What is the reason we want this functionality?

@tisto
Copy link
Member

@tisto tisto commented Jun 23, 2019

@robgietema @rodfersou shift-return should work in regular text tiles to being able to create quotes where you control the carriage return. No idea why that ended up in the description field.

@rodfersou
Copy link
Member Author

@rodfersou rodfersou commented Jun 24, 2019

@tisto @robgietema should be green after last change.

Please take a look at this comment where I sum up the state of the PR and tests #785 (comment)

@rodfersou
Copy link
Member Author

@rodfersou rodfersou commented Jun 24, 2019

finally green!

@@ -0,0 +1,286 @@
describe('Default Tiles functionality', () => {
Copy link
Member

@tisto tisto Jun 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no "default" tile. This is the "Text tile".

Copy link
Member Author

@rodfersou rodfersou Jun 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can review the structure of the tests later, for something that makes more sense.
right now I just put all tests in the same describe

}
});

it('Table Tile', () => {
Copy link
Member

@tisto tisto Jun 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no table tile in master yet. Therefore we should remove this test altogether.

Copy link
Member

@robgietema robgietema Jun 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes there is.

Copy link
Member

@tisto tisto Jun 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robgietema w00t. That's fantastic! Man, do you ever eat or sleep like other humans? ;)

@tisto tisto changed the title Makes shift+return create new line in Description field Makes shift+return create a new line in a text tile Jun 25, 2019
<Button
icon
basic
className="show-hidden-tiles"
Copy link
Member

@tisto tisto Jun 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rodfersou is "show-hidden-tiles" an existing class in our css?

Copy link
Member Author

@rodfersou rodfersou Jun 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I just use this class to fire the button in the test

CHANGELOG.md Outdated
@@ -4,6 +4,7 @@

### Added

- Makes shift+return create new line in Description field @rodfersou
Copy link
Member

@tisto tisto Jul 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rodfersou pls replace "Description field" with "Text tile".

Copy link
Member Author

@rodfersou rodfersou Jul 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@sneridagh sneridagh added this to the 4.0.0 milestone Aug 19, 2019
@tisto tisto added this to In progress in Volto 4 Final Sep 20, 2019
@tisto
Copy link
Member

@tisto tisto commented Sep 22, 2019

@rodfersou please try not to mix two different issues (shift+return, tiles tests) into one PR in the future. This makes things harder to review and merge.

@sneridagh
Copy link
Member

@sneridagh sneridagh commented Sep 22, 2019

@tisto FYI I've fixed a lot of issues and created a special redraftJS toHTML function to deal with this, I don't think that it's backported yet.

@tisto
Copy link
Member

@tisto tisto commented Sep 22, 2019

@rodfersou could you image to look into this again after Victor's refactoring? Maybe it is better to create a new branch for this.

@tisto
Copy link
Member

@tisto tisto commented Sep 25, 2019

@rodfersou it turns out we had to put quite some effort into make this work in our recent project. We have the code in one of our projects and we need to copy it over to Volto core.

@sneridagh
Copy link
Member

@sneridagh sneridagh commented Dec 9, 2019

Superseeded by #1044. @rodfersou I tried the ctrl+shift test but it failed :(
It would be great if we could extract this test and the other tests for the blocks that you did with this PR and update them and add it to the build. Not urgent though!

@sneridagh sneridagh moved this from In progress to Review in progress in Volto 4 Final Dec 9, 2019
@sneridagh sneridagh moved this from Review in progress to Reviewer approved in Volto 4 Final Dec 9, 2019
@sneridagh sneridagh moved this from Reviewer approved to Done in Volto 4 Final Dec 9, 2019
@tisto
Copy link
Member

@tisto tisto commented Dec 10, 2019

Fixed in master. Closing.

@tisto tisto closed this Dec 10, 2019
@tisto tisto deleted the shift-return-description branch Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Volto 4 Final
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants