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

Fixing calendar action in news creation #2096

Merged
merged 5 commits into from
Aug 28, 2024

Conversation

gnunicorn
Copy link
Contributor

While working on #2089 , I noticed that the way we construct the calendar-event-inline action is ... well, wrong. Instead of the typed CalendarReference we were using the LinkRef with some weird title and id properties as strings.

This PR corrects that with fallback support for the types we have already sent/published. See the new one following by an old one:

calendar-events-proper-types.mp4

To the reviewer:

  • This refactors the code to make it easier to add more types later as well as to use proper typing
  • Due to the fallback support needed, this also already contains a way to render regular links, though we don't have any construction code for it yet.
  • as I did that as a split out of Showing better Error messages #2089 this is probably harder to review. You only need to care about d5050d6 and following. But we have to wait with merging until we have the other one finalised anyways.

@gnunicorn gnunicorn added smooth UX minor UI/UX problems and tasks s-news News Section labels Aug 23, 2024
Copy link
Contributor

@kumarpalsinh25 kumarpalsinh25 left a comment

Choose a reason for hiding this comment

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

Review below commits which are specific to this PR.

In general changes looks good but there were few things that either I can't remember or understand well. Happy to proceed with merge after answering the queries.

Comment on lines 192 to 198
onTap: () async {
await ActerErrorDialog.show(
context: context,
error: e,
stack: s,
);
},
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

Comment on lines +5 to +17
static NewsReferencesType? fromStr(String typeStr) {
return values.asNameMap()[_toCamelCase(typeStr)];
}
}

String _toCamelCase(String s) {
final words = s
.split('-')
.map((w) =>
'${w.substring(0, 1).toUpperCase()}${w.substring(1).toLowerCase()}',)
.toList();
words[0] = words[0].toLowerCase();
return words.join();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite understand this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it rewrites strings from things-named-like-this to thingsNamedLikeThis

app/lib/features/news/widgets/news_item.dart Show resolved Hide resolved
app/lib/features/news/widgets/news_item.dart Show resolved Hide resolved
@gnunicorn gnunicorn merged commit 26d4d5c into main Aug 28, 2024
21 checks passed
@gnunicorn gnunicorn deleted the fixing-calendar-action-in-news-creation branch August 28, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s-news News Section smooth UX minor UI/UX problems and tasks
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants