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

Refactor textstructure #118

Merged
merged 13 commits into from
Jul 3, 2020
Merged

Refactor textstructure #118

merged 13 commits into from
Jul 3, 2020

Conversation

konradh
Copy link
Contributor

@konradh konradh commented Jun 18, 2020

No description provided.

@konradh konradh self-assigned this Jun 18, 2020
@konradh konradh marked this pull request as draft June 18, 2020 23:09
@konradh
Copy link
Contributor Author

konradh commented Jun 19, 2020

Extendable attributes should be improved on, currently there is only an option to disable extendability.

@konradh konradh marked this pull request as ready for review June 19, 2020 16:15
@frcroth
Copy link
Contributor

frcroth commented Jun 20, 2020

I would approve merge when Travis build is successfull and Coveage is at least 90.5% (still resulting in a drop of 1%). This does not have to be done by @konradh of course.

Copy link
Contributor

@sirkrypt0 sirkrypt0 left a comment

Choose a reason for hiding this comment

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

Generally speaking I like the new design, as it allows for more flexibility when creating new sorts of text structures.
I‘m unsure about the fact, that a TextStructure could hold multiple color/font etc. attributes as this is kinda counterintuitive and imo does not make sense at all. Furthermore this might be prone to issues when evaluating which style will be applied.

I was just wondering, what happens if a user creates a new attribute with the same symbol as our predefined ones. I think this will lead to errors and should be checked when creating a new structure by our user.

@sirkrypt0
Copy link
Contributor

(Review 1h just reading through all changes lol)

@frcroth frcroth added this to the Fifth Sprint milestone Jul 1, 2020
@frcroth
Copy link
Contributor

frcroth commented Jul 2, 2020

What is stopping us from merging this? The coverage?

@konradh
Copy link
Contributor Author

konradh commented Jul 2, 2020

I think this can be merged now.

@frcroth frcroth merged commit ed874d5 into dev Jul 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants